RFR (S): 8189797:Fix initializer lists in G1ConcurrentMark and G1CMTask

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

RFR (S): 8189797:Fix initializer lists in G1ConcurrentMark and G1CMTask

Thomas Schatzl
Hi all,

  can I have quick reviews for this change that fixes initializer lists
for the aforementioned classes?

It just rearranges their members so they reflect the actual
initialization order as mandated by the c++ standard. This is just
nicer for anyone trying to think about initialization order. There is
no actual bug fixed.

CR:
https://bugs.openjdk.java.net/browse/JDK-8189797
Webrev:
http://cr.openjdk.java.net/~tschatzl/8189797/webrev/
Testing:
local compilation

Thanks,
  Thomas
Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8189797:Fix initializer lists in G1ConcurrentMark and G1CMTask

Kim Barrett
> On Oct 23, 2017, at 12:02 PM, Thomas Schatzl <[hidden email]> wrote:
>
> Hi all,
>
>  can I have quick reviews for this change that fixes initializer lists
> for the aforementioned classes?
>
> It just rearranges their members so they reflect the actual
> initialization order as mandated by the c++ standard. This is just
> nicer for anyone trying to think about initialization order. There is
> no actual bug fixed.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8189797
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8189797/webrev/
> Testing:
> local compilation
>
> Thanks,
>  Thomas

I like this change.  Some time ago I tried the experiment of turning
on -Wreorder, and got lots of warnings.  I don't remember whether I
filed a bug for that, and JIRA seems to not be responding to me right
now.

Just a couple of very minor comments.  I don't need a new webrev for
these.

_cm_thread not commented in initializer list as being initialized
later.

When there's an initializer list, especially a long one like this, I
like to put the opening brace for the constructor body at the
beginning of its own line, in order to make it easier to spot where
the initializer list ends and the constructor body begins.

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8189797:Fix initializer lists in G1ConcurrentMark and G1CMTask

Kim Barrett
> On Oct 23, 2017, at 12:50 PM, Kim Barrett <[hidden email]> wrote:
> When there's an initializer list, especially a long one like this, I
> like to put the opening brace for the constructor body at the
> beginning of its own line, in order to make it easier to spot where
> the initializer list ends and the constructor body begins.

I forgot that for G1CMTask you moved the colon indicating the start of
the initializer list, but then left the indentation of the initializer
list unchanged, making it now inconsistent with others, including
G1ConcurrentMark in the same file.

I used to use the before-change style of G1CMTask (which makes the
initialier list to constructor body transition more obvious), but
changed to using always-cuddled ":" because that was more consistent
with other code I was seeing in Hotspot.

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8189797:Fix initializer lists in G1ConcurrentMark and G1CMTask

Thomas Schatzl
Hi Kim,

  thanks for your review.

On Mon, 2017-10-23 at 15:06 -0400, Kim Barrett wrote:
> I forgot that for G1CMTask you moved the colon indicating the start
> of
> the initializer list, but then left the indentation of the
> initializer
> list unchanged, making it now inconsistent with others, including
> G1ConcurrentMark in the same file.

All fixed I think. Since this is such a trivial change, assuming I do
not get more feedback, I will just push it in the next few days.

Webrevs:
http://cr.openjdk.java.net/~tschatzl/8189797/webrev.1 (full)
http://cr.openjdk.java.net/~tschatzl/8189797/webrev.0_to_1 (diff)

Thanks,
  Thomas