RFR: 8254050: HotSpot Style Guide should permit using the "override" virtual specifier

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8254050: HotSpot Style Guide should permit using the "override" virtual specifier

Kim Barrett-2
Please review and vote on this change to the HotSpot Style Guide to permit
the use of `override` virtual specifiers.  The virtual specifiers `override`
and `final` were added in C++11, and use of `final` is already permitted in
HotSpot code.

Using the `override` specifier provides error checking that the function is
indeed overriding a virtual function declared in a base class.  This can
prevent some often surprisingly difficult to spot bugs.

This is a modification of the Style Guide, so rough consensus among
the HotSpot Group members is required to make this change.  Only Group
members should vote for approval (via the github PR), though reasoned
objections or comments from anyone will be considered.  A decision on
this proposal will not be made before Tuesday 30-Mar-2021 at 12h00 UTC.

Since we're piggybacking on github PRs here, please use the PR review
process to approve (click on Review Changes > Approve), rather than
sending a "vote: yes" email reply that would be normal for a CFV.
Other responses can still use email of course.

-------------

Commit messages:
 - permit override specifier

Changes: https://git.openjdk.java.net/jdk/pull/3021/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3021&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8254050
  Stats: 8 lines in 1 file changed: 5 ins; 3 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3021.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3021/head:pull/3021

PR: https://git.openjdk.java.net/jdk/pull/3021
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8254050: HotSpot Style Guide should permit using the "override" virtual specifier

David Holmes-2
On Tue, 16 Mar 2021 03:44:56 GMT, Kim Barrett <[hidden email]> wrote:

> Please review and vote on this change to the HotSpot Style Guide to permit
> the use of `override` virtual specifiers.  The virtual specifiers `override`
> and `final` were added in C++11, and use of `final` is already permitted in
> HotSpot code.
>
> Using the `override` specifier provides error checking that the function is
> indeed overriding a virtual function declared in a base class.  This can
> prevent some often surprisingly difficult to spot bugs.
>
> This is a modification of the Style Guide, so rough consensus among
> the HotSpot Group members is required to make this change.  Only Group
> members should vote for approval (via the github PR), though reasoned
> objections or comments from anyone will be considered.  A decision on
> this proposal will not be made before Tuesday 30-Mar-2021 at 12h00 UTC.
>
> Since we're piggybacking on github PRs here, please use the PR review
> process to approve (click on Review Changes > Approve), rather than
> sending a "vote: yes" email reply that would be normal for a CFV.
> Other responses can still use email of course.

Fine by me.

Thanks,
David

-------------

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3021
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8254050: HotSpot Style Guide should permit using the "override" virtual specifier

Kim Barrett-2
On Tue, 16 Mar 2021 04:06:55 GMT, David Holmes <[hidden email]> wrote:

>> Please review and vote on this change to the HotSpot Style Guide to permit
>> the use of `override` virtual specifiers.  The virtual specifiers `override`
>> and `final` were added in C++11, and use of `final` is already permitted in
>> HotSpot code.
>>
>> Using the `override` specifier provides error checking that the function is
>> indeed overriding a virtual function declared in a base class.  This can
>> prevent some often surprisingly difficult to spot bugs.
>>
>> This is a modification of the Style Guide, so rough consensus among
>> the HotSpot Group members is required to make this change.  Only Group
>> members should vote for approval (via the github PR), though reasoned
>> objections or comments from anyone will be considered.  A decision on
>> this proposal will not be made before Tuesday 30-Mar-2021 at 12h00 UTC.
>>
>> Since we're piggybacking on github PRs here, please use the PR review
>> process to approve (click on Review Changes > Approve), rather than
>> sending a "vote: yes" email reply that would be normal for a CFV.
>> Other responses can still use email of course.
>
> Fine by me.
>
> Thanks,
> David

I forgot to mention that I didn't bother including the generated html in the PR, because it doesn't really add anything for the review, but will include it before integrating.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3021
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8254050: HotSpot Style Guide should permit using the "override" virtual specifier

John R Rose
In reply to this post by Kim Barrett-2
On Tue, 16 Mar 2021 03:44:56 GMT, Kim Barrett <[hidden email]> wrote:

> Please review and vote on this change to the HotSpot Style Guide to permit
> the use of `override` virtual specifiers.  The virtual specifiers `override`
> and `final` were added in C++11, and use of `final` is already permitted in
> HotSpot code.
>
> Using the `override` specifier provides error checking that the function is
> indeed overriding a virtual function declared in a base class.  This can
> prevent some often surprisingly difficult to spot bugs.
>
> This is a modification of the Style Guide, so rough consensus among
> the HotSpot Group members is required to make this change.  Only Group
> members should vote for approval (via the github PR), though reasoned
> objections or comments from anyone will be considered.  A decision on
> this proposal will not be made before Tuesday 30-Mar-2021 at 12h00 UTC.
>
> Since we're piggybacking on github PRs here, please use the PR review
> process to approve (click on Review Changes > Approve), rather than
> sending a "vote: yes" email reply that would be normal for a CFV.
> Other responses can still use email of course.

Marked as reviewed by jrose (Reviewer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/3021
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8254050: HotSpot Style Guide should permit using the "override" virtual specifier

Thomas Stuefe
In reply to this post by Kim Barrett-2
On Tue, 16 Mar 2021 03:44:56 GMT, Kim Barrett <[hidden email]> wrote:

> Please review and vote on this change to the HotSpot Style Guide to permit
> the use of `override` virtual specifiers.  The virtual specifiers `override`
> and `final` were added in C++11, and use of `final` is already permitted in
> HotSpot code.
>
> Using the `override` specifier provides error checking that the function is
> indeed overriding a virtual function declared in a base class.  This can
> prevent some often surprisingly difficult to spot bugs.
>
> This is a modification of the Style Guide, so rough consensus among
> the HotSpot Group members is required to make this change.  Only Group
> members should vote for approval (via the github PR), though reasoned
> objections or comments from anyone will be considered.  A decision on
> this proposal will not be made before Tuesday 30-Mar-2021 at 12h00 UTC.
>
> Since we're piggybacking on github PRs here, please use the PR review
> process to approve (click on Review Changes > Approve), rather than
> sending a "vote: yes" email reply that would be normal for a CFV.
> Other responses can still use email of course.

Looks good and makes sense.

-------------

Marked as reviewed by stuefe (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3021
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8254050: HotSpot Style Guide should permit using the "override" virtual specifier

Thomas Schatzl-4
In reply to this post by Kim Barrett-2
On Tue, 16 Mar 2021 03:44:56 GMT, Kim Barrett <[hidden email]> wrote:

> Please review and vote on this change to the HotSpot Style Guide to permit
> the use of `override` virtual specifiers.  The virtual specifiers `override`
> and `final` were added in C++11, and use of `final` is already permitted in
> HotSpot code.
>
> Using the `override` specifier provides error checking that the function is
> indeed overriding a virtual function declared in a base class.  This can
> prevent some often surprisingly difficult to spot bugs.
>
> This is a modification of the Style Guide, so rough consensus among
> the HotSpot Group members is required to make this change.  Only Group
> members should vote for approval (via the github PR), though reasoned
> objections or comments from anyone will be considered.  A decision on
> this proposal will not be made before Tuesday 30-Mar-2021 at 12h00 UTC.
>
> Since we're piggybacking on github PRs here, please use the PR review
> process to approve (click on Review Changes > Approve), rather than
> sending a "vote: yes" email reply that would be normal for a CFV.
> Other responses can still use email of course.

Marked as reviewed by tschatzl (Reviewer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/3021
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8254050: HotSpot Style Guide should permit using the "override" virtual specifier

Daniel D.Daugherty
In reply to this post by Kim Barrett-2
On Tue, 16 Mar 2021 03:44:56 GMT, Kim Barrett <[hidden email]> wrote:

> Please review and vote on this change to the HotSpot Style Guide to permit
> the use of `override` virtual specifiers.  The virtual specifiers `override`
> and `final` were added in C++11, and use of `final` is already permitted in
> HotSpot code.
>
> Using the `override` specifier provides error checking that the function is
> indeed overriding a virtual function declared in a base class.  This can
> prevent some often surprisingly difficult to spot bugs.
>
> This is a modification of the Style Guide, so rough consensus among
> the HotSpot Group members is required to make this change.  Only Group
> members should vote for approval (via the github PR), though reasoned
> objections or comments from anyone will be considered.  A decision on
> this proposal will not be made before Tuesday 30-Mar-2021 at 12h00 UTC.
>
> Since we're piggybacking on github PRs here, please use the PR review
> process to approve (click on Review Changes > Approve), rather than
> sending a "vote: yes" email reply that would be normal for a CFV.
> Other responses can still use email of course.

Marked as reviewed by dcubed (Reviewer).

doc/hotspot-style.md line 753:

> 751: ([n2928](http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2009/n2928.htm)),
> 752: ([n3206](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2010/n3206.htm)),
> 753: ([n3272](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2011/n3272.htm))

nit - The difference in case caught my attention:

    JTC1 <-> jtc1
    SC22 <-> sc22
    WG21 <-> wg21

any particular significance to the difference?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3021
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8254050: HotSpot Style Guide should permit using the "override" virtual specifier

Ioi Lam-2
In reply to this post by Kim Barrett-2
On Tue, 16 Mar 2021 03:44:56 GMT, Kim Barrett <[hidden email]> wrote:

> Please review and vote on this change to the HotSpot Style Guide to permit
> the use of `override` virtual specifiers.  The virtual specifiers `override`
> and `final` were added in C++11, and use of `final` is already permitted in
> HotSpot code.
>
> Using the `override` specifier provides error checking that the function is
> indeed overriding a virtual function declared in a base class.  This can
> prevent some often surprisingly difficult to spot bugs.
>
> This is a modification of the Style Guide, so rough consensus among
> the HotSpot Group members is required to make this change.  Only Group
> members should vote for approval (via the github PR), though reasoned
> objections or comments from anyone will be considered.  A decision on
> this proposal will not be made before Tuesday 30-Mar-2021 at 12h00 UTC.
>
> Since we're piggybacking on github PRs here, please use the PR review
> process to approve (click on Review Changes > Approve), rather than
> sending a "vote: yes" email reply that would be normal for a CFV.
> Other responses can still use email of course.

Marked as reviewed by iklam (Reviewer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/3021
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8254050: HotSpot Style Guide should permit using the "override" virtual specifier

Vladimir Kozlov-2
In reply to this post by Kim Barrett-2
On Tue, 16 Mar 2021 03:44:56 GMT, Kim Barrett <[hidden email]> wrote:

> Please review and vote on this change to the HotSpot Style Guide to permit
> the use of `override` virtual specifiers.  The virtual specifiers `override`
> and `final` were added in C++11, and use of `final` is already permitted in
> HotSpot code.
>
> Using the `override` specifier provides error checking that the function is
> indeed overriding a virtual function declared in a base class.  This can
> prevent some often surprisingly difficult to spot bugs.
>
> This is a modification of the Style Guide, so rough consensus among
> the HotSpot Group members is required to make this change.  Only Group
> members should vote for approval (via the github PR), though reasoned
> objections or comments from anyone will be considered.  A decision on
> this proposal will not be made before Tuesday 30-Mar-2021 at 12h00 UTC.
>
> Since we're piggybacking on github PRs here, please use the PR review
> process to approve (click on Review Changes > Approve), rather than
> sending a "vote: yes" email reply that would be normal for a CFV.
> Other responses can still use email of course.

Marked as reviewed by kvn (Reviewer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/3021
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8254050: HotSpot Style Guide should permit using the "override" virtual specifier [v2]

Kim Barrett-2
In reply to this post by Kim Barrett-2
> Please review and vote on this change to the HotSpot Style Guide to permit
> the use of `override` virtual specifiers.  The virtual specifiers `override`
> and `final` were added in C++11, and use of `final` is already permitted in
> HotSpot code.
>
> Using the `override` specifier provides error checking that the function is
> indeed overriding a virtual function declared in a base class.  This can
> prevent some often surprisingly difficult to spot bugs.
>
> This is a modification of the Style Guide, so rough consensus among
> the HotSpot Group members is required to make this change.  Only Group
> members should vote for approval (via the github PR), though reasoned
> objections or comments from anyone will be considered.  A decision on
> this proposal will not be made before Tuesday 30-Mar-2021 at 12h00 UTC.
>
> Since we're piggybacking on github PRs here, please use the PR review
> process to approve (click on Review Changes > Approve), rather than
> sending a "vote: yes" email reply that would be normal for a CFV.
> Other responses can still use email of course.

Kim Barrett has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision:

 - Merge branch 'master' into override_specifier
 - permit override specifier

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3021/files
  - new: https://git.openjdk.java.net/jdk/pull/3021/files/a48a6036..375a916e

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3021&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3021&range=00-01

  Stats: 42617 lines in 2058 files changed: 23969 ins; 9403 del; 9245 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3021.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3021/head:pull/3021

PR: https://git.openjdk.java.net/jdk/pull/3021
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8254050: HotSpot Style Guide should permit using the "override" virtual specifier [v2]

Kim Barrett-2
In reply to this post by Daniel D.Daugherty
On Tue, 16 Mar 2021 14:27:10 GMT, Daniel D. Daugherty <[hidden email]> wrote:

>> Kim Barrett has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision:
>>
>>  - Merge branch 'master' into override_specifier
>>  - permit override specifier
>
> doc/hotspot-style.md line 753:
>
>> 751: ([n2928](http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2009/n2928.htm)),
>> 752: ([n3206](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2010/n3206.htm)),
>> 753: ([n3272](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2011/n3272.htm))
>
> nit - The difference in case caught my attention:
>
>     JTC1 <-> jtc1
>     SC22 <-> sc22
>     WG21 <-> wg21
>
> any particular significance to the difference?

That's how they were written in the document I cut-and-pasted them from (https://gcc.gnu.org/projects/cxx-status.html).  Some of the links there seem to be to systems with case sensitive file-systems.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3021
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8254050: HotSpot Style Guide should permit using the "override" virtual specifier [v3]

Kim Barrett-2
In reply to this post by Vladimir Kozlov-2
On Tue, 23 Mar 2021 19:08:26 GMT, Vladimir Kozlov <[hidden email]> wrote:

>> Kim Barrett has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   rebuild hotspot-style.html
>
> Marked as reviewed by kvn (Reviewer).

There have been no issues raised with the proposed change, and plenty of support.  It has been approved by the HotSpot Lead (@vnkozlov ).  Thanks for all the reviews.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3021
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8254050: HotSpot Style Guide should permit using the "override" virtual specifier [v3]

Kim Barrett-2
In reply to this post by Kim Barrett-2
> Please review and vote on this change to the HotSpot Style Guide to permit
> the use of `override` virtual specifiers.  The virtual specifiers `override`
> and `final` were added in C++11, and use of `final` is already permitted in
> HotSpot code.
>
> Using the `override` specifier provides error checking that the function is
> indeed overriding a virtual function declared in a base class.  This can
> prevent some often surprisingly difficult to spot bugs.
>
> This is a modification of the Style Guide, so rough consensus among
> the HotSpot Group members is required to make this change.  Only Group
> members should vote for approval (via the github PR), though reasoned
> objections or comments from anyone will be considered.  A decision on
> this proposal will not be made before Tuesday 30-Mar-2021 at 12h00 UTC.
>
> Since we're piggybacking on github PRs here, please use the PR review
> process to approve (click on Review Changes > Approve), rather than
> sending a "vote: yes" email reply that would be normal for a CFV.
> Other responses can still use email of course.

Kim Barrett has updated the pull request incrementally with one additional commit since the last revision:

  rebuild hotspot-style.html

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3021/files
  - new: https://git.openjdk.java.net/jdk/pull/3021/files/375a916e..301a604d

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3021&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3021&range=01-02

  Stats: 2 lines in 1 file changed: 1 ins; 1 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3021.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3021/head:pull/3021

PR: https://git.openjdk.java.net/jdk/pull/3021
Reply | Threaded
Open this post in threaded view
|

Integrated: 8254050: HotSpot Style Guide should permit using the "override" virtual specifier

Kim Barrett-2
In reply to this post by Kim Barrett-2
On Tue, 16 Mar 2021 03:44:56 GMT, Kim Barrett <[hidden email]> wrote:

> Please review and vote on this change to the HotSpot Style Guide to permit
> the use of `override` virtual specifiers.  The virtual specifiers `override`
> and `final` were added in C++11, and use of `final` is already permitted in
> HotSpot code.
>
> Using the `override` specifier provides error checking that the function is
> indeed overriding a virtual function declared in a base class.  This can
> prevent some often surprisingly difficult to spot bugs.
>
> This is a modification of the Style Guide, so rough consensus among
> the HotSpot Group members is required to make this change.  Only Group
> members should vote for approval (via the github PR), though reasoned
> objections or comments from anyone will be considered.  A decision on
> this proposal will not be made before Tuesday 30-Mar-2021 at 12h00 UTC.
>
> Since we're piggybacking on github PRs here, please use the PR review
> process to approve (click on Review Changes > Approve), rather than
> sending a "vote: yes" email reply that would be normal for a CFV.
> Other responses can still use email of course.

This pull request has now been integrated.

Changeset: 07806669
Author:    Kim Barrett <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/07806669
Stats:     10 lines in 2 files changed: 6 ins; 4 del; 0 mod

8254050: HotSpot Style Guide should permit using the "override" virtual specifier

Reviewed-by: dholmes, jrose, stuefe, tschatzl, dcubed, iklam, kvn

-------------

PR: https://git.openjdk.java.net/jdk/pull/3021