Discussion:
RFR (L) 8214417: Add space after/before {} in vmTestbase/nsk/jvmti/[A-I] tests
(too old to reply)
JC Beyler
2018-11-28 19:20:08 UTC
Permalink
Hi all,

When working on a previous clean-up (JDK-8212771), I was asked to clean-up
also spaces around {}.

Here is the first batch out of 2 to fix these cases. Let me know what you
think.

Webrev: http://cr.openjdk.java.net/~jcbeyler/8214417/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8214417

Thanks for your reviews :-),
Jc
Alex Menkov
2018-11-28 23:43:55 UTC
Permalink
Hi Jc,

In the JDK-8212771 review thread cleanup for "{}" was requested for
statements like
test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/MethodBind/JvmtiTest/JvmtiTest.cpp,

+#define JVMTI_ERROR_CHECK(str,res) if (res != JVMTI_ERROR_NONE) {
printf(str); printf("%d\n",res); return res;}

+#define JVMTI_ERROR_CHECK_EXPECTED_ERROR(str,res,err) if (res != err) {
printf(str); printf("unexpected error %d\n",res); return res;}

I.e. something like ";}" -> "; }"


I don't think changes like

-static jvmtiEvent testEvents[] = {JVMTI_EVENT_THREAD_START,
JVMTI_EVENT_THREAD_END};
+static jvmtiEvent testEvents[] = { JVMTI_EVENT_THREAD_START,
JVMTI_EVENT_THREAD_END };

are required.

--alex
Post by JC Beyler
Hi all,
When working on a previous clean-up (JDK-8212771), I was asked to
clean-up also spaces around {}.
Here is the first batch out of 2 to fix these cases. Let me know what
you think.
Webrev: http://cr.openjdk.java.net/~jcbeyler/8214417/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8214417
Thanks for your reviews :-),
Jc
s***@oracle.com
2018-11-29 00:05:07 UTC
Permalink
Post by Alex Menkov
Hi Jc,
In the JDK-8212771 review thread cleanup for "{}" was requested for
statements like
test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/MethodBind/JvmtiTest/JvmtiTest.cpp,
+#define JVMTI_ERROR_CHECK(str,res) if (res != JVMTI_ERROR_NONE) {
printf(str); printf("%d\n",res); return res;}
+#define JVMTI_ERROR_CHECK_EXPECTED_ERROR(str,res,err) if (res != err)
{ printf(str); printf("unexpected error %d\n",res); return res;}
I.e. something like ";}" -> "; }"
I don't think changes like
-static jvmtiEvent testEvents[] = {JVMTI_EVENT_THREAD_START,
JVMTI_EVENT_THREAD_END};
+static jvmtiEvent testEvents[] = { JVMTI_EVENT_THREAD_START,
JVMTI_EVENT_THREAD_END };
are required.
It is better to have it - rules are rules.

Thanks,
Serguei
Post by Alex Menkov
--alex
Post by JC Beyler
Hi all,
When working on a previous clean-up (JDK-8212771), I was asked to
clean-up also spaces around {}.
Here is the first batch out of 2 to fix these cases. Let me know what
you think.
Webrev: http://cr.openjdk.java.net/~jcbeyler/8214417/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8214417
Thanks for your reviews :-),
Jc
Alex Menkov
2018-11-29 00:18:00 UTC
Permalink
I don't see such rule (I suppose
https://wiki.openjdk.java.net/display/HotSpot/StyleGuide is correct link?)

--alex
Post by s***@oracle.com
Post by Alex Menkov
Hi Jc,
In the JDK-8212771 review thread cleanup for "{}" was requested for
statements like
test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/MethodBind/JvmtiTest/JvmtiTest.cpp,
+#define JVMTI_ERROR_CHECK(str,res) if (res != JVMTI_ERROR_NONE) {
printf(str); printf("%d\n",res); return res;}
+#define JVMTI_ERROR_CHECK_EXPECTED_ERROR(str,res,err) if (res != err)
{ printf(str); printf("unexpected error %d\n",res); return res;}
I.e. something like ";}" -> "; }"
I don't think changes like
-static jvmtiEvent testEvents[] = {JVMTI_EVENT_THREAD_START,
JVMTI_EVENT_THREAD_END};
+static jvmtiEvent testEvents[] = { JVMTI_EVENT_THREAD_START,
JVMTI_EVENT_THREAD_END };
are required.
It is better to have it - rules are rules.
Thanks,
Serguei
Post by Alex Menkov
--alex
Post by JC Beyler
Hi all,
When working on a previous clean-up (JDK-8212771), I was asked to
clean-up also spaces around {}.
Here is the first batch out of 2 to fix these cases. Let me know what
you think.
Webrev: http://cr.openjdk.java.net/~jcbeyler/8214417/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8214417
Thanks for your reviews :-),
Jc
s***@oracle.com
2018-11-29 00:44:08 UTC
Permalink
There are some implicit rules, like unification and consistency.
We want a space or new line after '{' and before '}'.
Why this case needs to have an exception?

Thanks,
Serguei
Post by Alex Menkov
I don't see such rule (I suppose
https://wiki.openjdk.java.net/display/HotSpot/StyleGuide is correct link?)
--alex
Post by s***@oracle.com
Post by Alex Menkov
Hi Jc,
In the JDK-8212771 review thread cleanup for "{}" was requested for
statements like
test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/MethodBind/JvmtiTest/JvmtiTest.cpp,
+#define JVMTI_ERROR_CHECK(str,res) if (res != JVMTI_ERROR_NONE) {
printf(str); printf("%d\n",res); return res;}
+#define JVMTI_ERROR_CHECK_EXPECTED_ERROR(str,res,err) if (res !=
err) { printf(str); printf("unexpected error %d\n",res); return res;}
I.e. something like ";}" -> "; }"
I don't think changes like
-static jvmtiEvent testEvents[] = {JVMTI_EVENT_THREAD_START,
JVMTI_EVENT_THREAD_END};
+static jvmtiEvent testEvents[] = { JVMTI_EVENT_THREAD_START,
JVMTI_EVENT_THREAD_END };
are required.
It is better to have it - rules are rules.
Thanks,
Serguei
Post by Alex Menkov
--alex
Post by JC Beyler
Hi all,
When working on a previous clean-up (JDK-8212771), I was asked to
clean-up also spaces around {}.
Here is the first batch out of 2 to fix these cases. Let me know
what you think.
Webrev: http://cr.openjdk.java.net/~jcbeyler/8214417/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8214417
Thanks for your reviews :-),
Jc
Alex Menkov
2018-11-29 02:18:37 UTC
Permalink
Hi Serguei,
Post by s***@oracle.com
There are some implicit rules, like unification and consistency.
We want a space or new line after '{' and before '}'.
I believe this rule is about braces for code blocks (new line after "{",
"}" on a separate line), but this are array initializers. And to me for
1-line array initializers spaces do not improve readability.

So to me this looks like the last item from "Whitespace" is applicable here:
<cite>
Try not to change whitespace unless it improves readability or
consistency. (Different editors indent differently, and spurious
indentation changes will just make integrations more difficult.)
</cite>

--alex
Post by s***@oracle.com
Why this case needs to have an exception?
Thanks,
Serguei
Post by Alex Menkov
I don't see such rule (I suppose
https://wiki.openjdk.java.net/display/HotSpot/StyleGuide is correct link?)
--alex
Post by s***@oracle.com
Post by Alex Menkov
Hi Jc,
In the JDK-8212771 review thread cleanup for "{}" was requested for
statements like
test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/MethodBind/JvmtiTest/JvmtiTest.cpp,
+#define JVMTI_ERROR_CHECK(str,res) if (res != JVMTI_ERROR_NONE) {
printf(str); printf("%d\n",res); return res;}
+#define JVMTI_ERROR_CHECK_EXPECTED_ERROR(str,res,err) if (res !=
err) { printf(str); printf("unexpected error %d\n",res); return res;}
I.e. something like ";}" -> "; }"
I don't think changes like
-static jvmtiEvent testEvents[] = {JVMTI_EVENT_THREAD_START,
JVMTI_EVENT_THREAD_END};
+static jvmtiEvent testEvents[] = { JVMTI_EVENT_THREAD_START,
JVMTI_EVENT_THREAD_END };
are required.
It is better to have it - rules are rules.
Thanks,
Serguei
Post by Alex Menkov
--alex
Post by JC Beyler
Hi all,
When working on a previous clean-up (JDK-8212771), I was asked to
clean-up also spaces around {}.
Here is the first batch out of 2 to fix these cases. Let me know
what you think.
Webrev: http://cr.openjdk.java.net/~jcbeyler/8214417/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8214417
Thanks for your reviews :-),
Jc
JC Beyler
2018-11-29 03:44:27 UTC
Permalink
Hi both,

I actually am letting you both decide which you would rather do :-).

For what it's worth, for *cpp files:
A) there are 83 files in vmTestbase that have on the same line { and }
where } does not have a space before (659 cases of {} in the vmTestbase
(grep "{.*[^ {]}")
- there are 98 files in all (including the vmTestbase files) in the
whole test/hotspot directory () (with 790 cases)
- there are 281 files that contain it in the whole jdk tree, with
1776 cases in all (so 10% of the files contain at least one case)

(Note I'm not counting the number of times grep "{[^ }].*}" shows up here

B) there are 10 files in vmTestbase that have on the same line { and ;}
where ;} does not have a space between (19 cases of {} in the vmTestbase
(grep "{.*;}")
- there are 10 files in all (including the vmTestbase files) in the
whole test/hotspot directory (19 cases in all); so all are in the vmTestbase
- there are 44 files that contain it in the whole jdk tree, with
111 cases in all

(A) would close 821822 in general and we could determine if you wanted it
across the codebase, just for the vmTestbase or for the hotspot tests in
general
(B) is a much smaller clean-up, we could again do it across the codebase
with 44 files touched; or just in the vmTestbase since the hotspot tests do
not have the case except there.
(C) Not do it :)

What would you prefer I do ? I have an awk script for (A), a simple sed
command for (B), a few clicks to close the bugs in JBS for (C); so it's
easy either way :).
Jc
Post by Alex Menkov
Hi Serguei,
Post by s***@oracle.com
There are some implicit rules, like unification and consistency.
We want a space or new line after '{' and before '}'.
I believe this rule is about braces for code blocks (new line after "{",
"}" on a separate line), but this are array initializers. And to me for
1-line array initializers spaces do not improve readability.
<cite>
Try not to change whitespace unless it improves readability or
consistency. (Different editors indent differently, and spurious
indentation changes will just make integrations more difficult.)
</cite>
--alex
Post by s***@oracle.com
Why this case needs to have an exception?
Thanks,
Serguei
Post by Alex Menkov
I don't see such rule (I suppose
https://wiki.openjdk.java.net/display/HotSpot/StyleGuide is correct link?)
--alex
Post by s***@oracle.com
Post by Alex Menkov
Hi Jc,
In the JDK-8212771 review thread cleanup for "{}" was requested for
statements like
test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/MethodBind/JvmtiTest/JvmtiTest.cpp,
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
+#define JVMTI_ERROR_CHECK(str,res) if (res != JVMTI_ERROR_NONE) {
printf(str); printf("%d\n",res); return res;}
+#define JVMTI_ERROR_CHECK_EXPECTED_ERROR(str,res,err) if (res !=
err) { printf(str); printf("unexpected error %d\n",res); return res;}
I.e. something like ";}" -> "; }"
I don't think changes like
-static jvmtiEvent testEvents[] = {JVMTI_EVENT_THREAD_START,
JVMTI_EVENT_THREAD_END};
+static jvmtiEvent testEvents[] = { JVMTI_EVENT_THREAD_START,
JVMTI_EVENT_THREAD_END };
are required.
It is better to have it - rules are rules.
Thanks,
Serguei
Post by Alex Menkov
--alex
Post by JC Beyler
Hi all,
When working on a previous clean-up (JDK-8212771), I was asked to
clean-up also spaces around {}.
Here is the first batch out of 2 to fix these cases. Let me know
what you think.
Webrev: http://cr.openjdk.java.net/~jcbeyler/8214417/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8214417
Thanks for your reviews :-),
Jc
--
Thanks,
Jc
s***@oracle.com
2018-11-29 10:29:24 UTC
Permalink
Hi Alex,
Post by Alex Menkov
Hi Serguei,
Post by s***@oracle.com
There are some implicit rules, like unification and consistency.
We want a space or new line after '{' and before '}'.
I believe this rule is about braces for code blocks (new line after
"{", "}" on a separate line), but this are array initializers.
Array initializers are blocs as well.
It make it a base for unification.
Post by Alex Menkov
And to me for 1-line array initializers spaces do not improve readability.
I politely disagree, at least, they improve it for me. :)

We can do one simple test.
What suggestion would make the Jc's awk script simpler?
If yours then I'm out.
Otherwise, why does it make simpler for script but not for humans?

Also, we can wait for one more opinion.
Post by Alex Menkov
<cite>
Try not to change whitespace unless it improves readability or
consistency. (Different editors indent differently, and spurious
indentation changes will just make integrations more difficult.)
</cite>
It was Okay to break this rule when we decided to fix spacing over all
the tests.

Thanks,
Serguei
Post by Alex Menkov
--alex
Post by s***@oracle.com
Why this case needs to have an exception?
Thanks,
Serguei
Post by Alex Menkov
I don't see such rule (I suppose
https://wiki.openjdk.java.net/display/HotSpot/StyleGuide is correct link?)
--alex
Post by s***@oracle.com
Post by Alex Menkov
Hi Jc,
In the JDK-8212771 review thread cleanup for "{}" was requested
for statements like
test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/MethodBind/JvmtiTest/JvmtiTest.cpp,
+#define JVMTI_ERROR_CHECK(str,res) if (res != JVMTI_ERROR_NONE) {
printf(str); printf("%d\n",res); return res;}
+#define JVMTI_ERROR_CHECK_EXPECTED_ERROR(str,res,err) if (res !=
err) { printf(str); printf("unexpected error %d\n",res); return res;}
I.e. something like ";}" -> "; }"
I don't think changes like
-static jvmtiEvent testEvents[] = {JVMTI_EVENT_THREAD_START,
JVMTI_EVENT_THREAD_END};
+static jvmtiEvent testEvents[] = { JVMTI_EVENT_THREAD_START,
JVMTI_EVENT_THREAD_END };
are required.
It is better to have it - rules are rules.
Thanks,
Serguei
Post by Alex Menkov
--alex
Post by JC Beyler
Hi all,
When working on a previous clean-up (JDK-8212771), I was asked to
clean-up also spaces around {}.
Here is the first batch out of 2 to fix these cases. Let me know
what you think.
Webrev: http://cr.openjdk.java.net/~jcbeyler/8214417/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8214417
Thanks for your reviews :-),
Jc
David Holmes
2018-11-29 10:48:33 UTC
Permalink
I vote for the spaces around { and }

I would write:

int foo() { return 1; }

so would also write:

static jvmtiEvent testEvents[] = { JVMTI_EVENT_THREAD_START,
JVMTI_EVENT_THREAD_END };

Cheers,
David
Post by s***@oracle.com
Hi Alex,
Post by Alex Menkov
Hi Serguei,
Post by s***@oracle.com
There are some implicit rules, like unification and consistency.
We want a space or new line after '{' and before '}'.
I believe this rule is about braces for code blocks (new line after
"{", "}" on a separate line), but this are array initializers.
Array initializers are blocs as well.
It make it a base for unification.
Post by Alex Menkov
And to me for 1-line array initializers spaces do not improve
readability.
I politely disagree, at least, they improve it for me. :)
We can do one simple test.
What suggestion would make the Jc's awk script simpler?
If yours then I'm out.
Otherwise, why does it make simpler for script but not for humans?
Also, we can wait for one more opinion.
Post by Alex Menkov
<cite>
Try not to change whitespace unless it improves readability or
consistency. (Different editors indent differently, and spurious
indentation changes will just make integrations more difficult.)
</cite>
It was Okay to break this rule when we decided to fix spacing over all
the tests.
Thanks,
Serguei
Post by Alex Menkov
--alex
Post by s***@oracle.com
Why this case needs to have an exception?
Thanks,
Serguei
Post by Alex Menkov
I don't see such rule (I suppose
https://wiki.openjdk.java.net/display/HotSpot/StyleGuide is correct link?)
--alex
Post by s***@oracle.com
Post by Alex Menkov
Hi Jc,
In the JDK-8212771 review thread cleanup for "{}" was requested
for statements like
test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/MethodBind/JvmtiTest/JvmtiTest.cpp,
+#define JVMTI_ERROR_CHECK(str,res) if (res != JVMTI_ERROR_NONE) {
printf(str); printf("%d\n",res); return res;}
+#define JVMTI_ERROR_CHECK_EXPECTED_ERROR(str,res,err) if (res !=
err) { printf(str); printf("unexpected error %d\n",res); return res;}
I.e. something like ";}" -> "; }"
I don't think changes like
-static jvmtiEvent testEvents[] = {JVMTI_EVENT_THREAD_START,
JVMTI_EVENT_THREAD_END};
+static jvmtiEvent testEvents[] = { JVMTI_EVENT_THREAD_START,
JVMTI_EVENT_THREAD_END };
are required.
It is better to have it - rules are rules.
Thanks,
Serguei
Post by Alex Menkov
--alex
Post by JC Beyler
Hi all,
When working on a previous clean-up (JDK-8212771), I was asked to
clean-up also spaces around {}.
Here is the first batch out of 2 to fix these cases. Let me know
what you think.
Webrev: http://cr.openjdk.java.net/~jcbeyler/8214417/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8214417
Thanks for your reviews :-),
Jc
Daniel D. Daugherty
2018-11-29 19:32:18 UTC
Permalink
I would also write code like what David shows... :-)

Dan
Post by David Holmes
I vote for the spaces around { and }
int foo() { return 1; }
static jvmtiEvent testEvents[] = { JVMTI_EVENT_THREAD_START,
JVMTI_EVENT_THREAD_END };
Cheers,
David
Post by s***@oracle.com
Hi Alex,
Post by Alex Menkov
Hi Serguei,
Post by s***@oracle.com
There are some implicit rules, like unification and consistency.
We want a space or new line after '{' and before '}'.
I believe this rule is about braces for code blocks (new line after
"{", "}" on a separate line), but this are array initializers.
Array initializers are blocs as well.
It make it a base for unification.
Post by Alex Menkov
And to me for 1-line array initializers spaces do not improve readability.
I politely disagree, at least, they improve it for me. :)
We can do one simple test.
What suggestion would make the Jc's awk script simpler?
If yours then I'm out.
Otherwise, why does it make simpler for script but not for humans?
Also, we can wait for one more opinion.
Post by Alex Menkov
So to me this looks like the last item from "Whitespace" is
<cite>
Try not to change whitespace unless it improves readability or
consistency. (Different editors indent differently, and spurious
indentation changes will just make integrations more difficult.)
</cite>
It was Okay to break this rule when we decided to fix spacing over
all the tests.
Thanks,
Serguei
Post by Alex Menkov
--alex
Post by s***@oracle.com
Why this case needs to have an exception?
Thanks,
Serguei
Post by Alex Menkov
I don't see such rule (I suppose
https://wiki.openjdk.java.net/display/HotSpot/StyleGuide is correct link?)
--alex
Post by s***@oracle.com
Post by Alex Menkov
Hi Jc,
In the JDK-8212771 review thread cleanup for "{}" was requested
for statements like
test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/MethodBind/JvmtiTest/JvmtiTest.cpp,
+#define JVMTI_ERROR_CHECK(str,res) if (res != JVMTI_ERROR_NONE)
{ printf(str); printf("%d\n",res); return res;}
+#define JVMTI_ERROR_CHECK_EXPECTED_ERROR(str,res,err) if (res
!= err) { printf(str); printf("unexpected error %d\n",res);
return res;}
I.e. something like ";}" -> "; }"
I don't think changes like
-static jvmtiEvent testEvents[] = {JVMTI_EVENT_THREAD_START,
JVMTI_EVENT_THREAD_END};
+static jvmtiEvent testEvents[] = { JVMTI_EVENT_THREAD_START,
JVMTI_EVENT_THREAD_END };
are required.
It is better to have it - rules are rules.
Thanks,
Serguei
Post by Alex Menkov
--alex
Post by JC Beyler
Hi all,
When working on a previous clean-up (JDK-8212771), I was asked
to clean-up also spaces around {}.
Here is the first batch out of 2 to fix these cases. Let me
know what you think.
Webrev: http://cr.openjdk.java.net/~jcbeyler/8214417/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8214417
Thanks for your reviews :-),
Jc
Alex Menkov
2018-11-29 22:26:34 UTC
Permalink
Ok, guys, you win :)

About the fix:
Looks like only 1-line initializers are fixed, so now we have mixed
style in some files like:

ClearFieldAccessWatch/clrfldw001/clrfldw001.cpp (lines 52-53 are not
updated):

NULL },
51 { "nsk/jvmti/ClearFieldAccessWatch/clrfldw001", "fld1", "I", 1,
NULL },
52 {"nsk/jvmti/ClearFieldAccessWatch/clrfldw001", "fld2",
53 "Lnsk/jvmti/ClearFieldAccessWatch/clrfldw001a;", 0, NULL},
54 { "nsk/jvmti/ClearFieldAccessWatch/clrfldw001a", "fld3", "[I", 0,
NULL },


The same in ClearFieldModificationWatch/clrfmodw001/clrfmodw001.cpp
(lines 52-53):
51 { "nsk/jvmti/ClearFieldModificationWatch/clrfmodw001", "fld1",
"I", 1, NULL },
52 {"nsk/jvmti/ClearFieldModificationWatch/clrfmodw001", "fld2",
53 "Lnsk/jvmti/ClearFieldModificationWatch/clrfmodw001a;", 0, NULL},
54 { "nsk/jvmti/ClearFieldModificationWatch/clrfmodw001a", "fld3",
"[I", 0, NULL },


GetClassSignature/getclsig006/getclsig006.cpp (44-51)
43 { "getclsig006", "Lnsk/jvmti/GetClassSignature/getclsig006;",
"NULL" },
44 {"getclsig006b", "Lnsk/jvmti/GetClassSignature/getclsig006b;",
45 "<L:Ljava/lang/String;>Ljava/lang/Object;"},
46 {"getclsig006c", "Lnsk/jvmti/GetClassSignature/getclsig006c;",
47 "<A:Ljava/lang/Object;B:Ljava/lang/Integer;>Ljava/lang/Object;"},

(and others files)

--alex
Post by Daniel D. Daugherty
I would also write code like what David shows... :-)
Dan
Post by David Holmes
I vote for the spaces around { and }
int foo() { return 1; }
static jvmtiEvent testEvents[] = { JVMTI_EVENT_THREAD_START,
JVMTI_EVENT_THREAD_END };
Cheers,
David
Post by s***@oracle.com
Hi Alex,
Post by Alex Menkov
Hi Serguei,
Post by s***@oracle.com
There are some implicit rules, like unification and consistency.
We want a space or new line after '{' and before '}'.
I believe this rule is about braces for code blocks (new line after
"{", "}" on a separate line), but this are array initializers.
Array initializers are blocs as well.
It make it a base for unification.
Post by Alex Menkov
And to me for 1-line array initializers spaces do not improve readability.
I politely disagree, at least, they improve it for me. :)
We can do one simple test.
What suggestion would make the Jc's awk script simpler?
If yours then I'm out.
Otherwise, why does it make simpler for script but not for humans?
Also, we can wait for one more opinion.
Post by Alex Menkov
So to me this looks like the last item from "Whitespace" is
<cite>
Try not to change whitespace unless it improves readability or
consistency. (Different editors indent differently, and spurious
indentation changes will just make integrations more difficult.)
</cite>
It was Okay to break this rule when we decided to fix spacing over
all the tests.
Thanks,
Serguei
Post by Alex Menkov
--alex
Post by s***@oracle.com
Why this case needs to have an exception?
Thanks,
Serguei
Post by Alex Menkov
I don't see such rule (I suppose
https://wiki.openjdk.java.net/display/HotSpot/StyleGuide is correct link?)
--alex
Post by s***@oracle.com
Post by Alex Menkov
Hi Jc,
In the JDK-8212771 review thread cleanup for "{}" was requested
for statements like
test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/MethodBind/JvmtiTest/JvmtiTest.cpp,
+#define JVMTI_ERROR_CHECK(str,res) if (res != JVMTI_ERROR_NONE)
{ printf(str); printf("%d\n",res); return res;}
+#define JVMTI_ERROR_CHECK_EXPECTED_ERROR(str,res,err) if (res
!= err) { printf(str); printf("unexpected error %d\n",res);
return res;}
I.e. something like ";}" -> "; }"
I don't think changes like
-static jvmtiEvent testEvents[] = {JVMTI_EVENT_THREAD_START,
JVMTI_EVENT_THREAD_END};
+static jvmtiEvent testEvents[] = { JVMTI_EVENT_THREAD_START,
JVMTI_EVENT_THREAD_END };
are required.
It is better to have it - rules are rules.
Thanks,
Serguei
Post by Alex Menkov
--alex
Post by JC Beyler
Hi all,
When working on a previous clean-up (JDK-8212771), I was asked
to clean-up also spaces around {}.
Here is the first batch out of 2 to fix these cases. Let me
know what you think.
Webrev: http://cr.openjdk.java.net/~jcbeyler/8214417/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8214417
Thanks for your reviews :-),
Jc
JC Beyler
2018-11-29 23:00:37 UTC
Permalink
Hi Alex,

Yes that is true, I had initially only done the one-liner {} because I was
unsure about the cases above :-)

Here is a webrev handling all cases for those 40 files:
Webrev: http://cr.openjdk.java.net/~jcbeyler/8214417/webrev.01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8214417

This was tested on my dev machine for all changed tests.

Thanks!
Jc
Post by Alex Menkov
Ok, guys, you win :)
Looks like only 1-line initializers are fixed, so now we have mixed
ClearFieldAccessWatch/clrfldw001/clrfldw001.cpp (lines 52-53 are not
NULL },
51 { "nsk/jvmti/ClearFieldAccessWatch/clrfldw001", "fld1", "I", 1,
NULL },
52 {"nsk/jvmti/ClearFieldAccessWatch/clrfldw001", "fld2",
53 "Lnsk/jvmti/ClearFieldAccessWatch/clrfldw001a;", 0, NULL},
54 { "nsk/jvmti/ClearFieldAccessWatch/clrfldw001a", "fld3", "[I", 0,
NULL },
The same in ClearFieldModificationWatch/clrfmodw001/clrfmodw001.cpp
51 { "nsk/jvmti/ClearFieldModificationWatch/clrfmodw001", "fld1",
"I", 1, NULL },
52 {"nsk/jvmti/ClearFieldModificationWatch/clrfmodw001", "fld2",
53 "Lnsk/jvmti/ClearFieldModificationWatch/clrfmodw001a;", 0, NULL},
54 { "nsk/jvmti/ClearFieldModificationWatch/clrfmodw001a", "fld3",
"[I", 0, NULL },
GetClassSignature/getclsig006/getclsig006.cpp (44-51)
43 { "getclsig006", "Lnsk/jvmti/GetClassSignature/getclsig006;",
"NULL" },
44 {"getclsig006b", "Lnsk/jvmti/GetClassSignature/getclsig006b;",
45 "<L:Ljava/lang/String;>Ljava/lang/Object;"},
46 {"getclsig006c", "Lnsk/jvmti/GetClassSignature/getclsig006c;",
47
"<A:Ljava/lang/Object;B:Ljava/lang/Integer;>Ljava/lang/Object;"},
(and others files)
--alex
Post by Daniel D. Daugherty
I would also write code like what David shows... :-)
Dan
Post by David Holmes
I vote for the spaces around { and }
int foo() { return 1; }
static jvmtiEvent testEvents[] = { JVMTI_EVENT_THREAD_START,
JVMTI_EVENT_THREAD_END };
Cheers,
David
Post by s***@oracle.com
Hi Alex,
Post by Alex Menkov
Hi Serguei,
Post by s***@oracle.com
There are some implicit rules, like unification and consistency.
We want a space or new line after '{' and before '}'.
I believe this rule is about braces for code blocks (new line after
"{", "}" on a separate line), but this are array initializers.
Array initializers are blocs as well.
It make it a base for unification.
Post by Alex Menkov
And to me for 1-line array initializers spaces do not improve readability.
I politely disagree, at least, they improve it for me. :)
We can do one simple test.
What suggestion would make the Jc's awk script simpler?
If yours then I'm out.
Otherwise, why does it make simpler for script but not for humans?
Also, we can wait for one more opinion.
Post by Alex Menkov
<cite>
Try not to change whitespace unless it improves readability or
consistency. (Different editors indent differently, and spurious
indentation changes will just make integrations more difficult.)
</cite>
It was Okay to break this rule when we decided to fix spacing over
all the tests.
Thanks,
Serguei
Post by Alex Menkov
--alex
Post by s***@oracle.com
Why this case needs to have an exception?
Thanks,
Serguei
Post by Alex Menkov
I don't see such rule (I suppose
https://wiki.openjdk.java.net/display/HotSpot/StyleGuide is correct link?)
--alex
Post by s***@oracle.com
Post by Alex Menkov
Hi Jc,
In the JDK-8212771 review thread cleanup for "{}" was requested
for statements like
test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/MethodBind/JvmtiTest/JvmtiTest.cpp,
Post by Daniel D. Daugherty
Post by David Holmes
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
+#define JVMTI_ERROR_CHECK(str,res) if (res != JVMTI_ERROR_NONE)
{ printf(str); printf("%d\n",res); return res;}
+#define JVMTI_ERROR_CHECK_EXPECTED_ERROR(str,res,err) if (res
!= err) { printf(str); printf("unexpected error %d\n",res);
return res;}
I.e. something like ";}" -> "; }"
I don't think changes like
-static jvmtiEvent testEvents[] = {JVMTI_EVENT_THREAD_START,
JVMTI_EVENT_THREAD_END};
+static jvmtiEvent testEvents[] = { JVMTI_EVENT_THREAD_START,
JVMTI_EVENT_THREAD_END };
are required.
It is better to have it - rules are rules.
Thanks,
Serguei
Post by Alex Menkov
--alex
Post by JC Beyler
Hi all,
When working on a previous clean-up (JDK-8212771), I was asked
to clean-up also spaces around {}.
Here is the first batch out of 2 to fix these cases. Let me
know what you think.
Webrev: http://cr.openjdk.java.net/~jcbeyler/8214417/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8214417
Thanks for your reviews :-),
Jc
--
Thanks,
Jc
Alex Menkov
2018-11-29 23:57:19 UTC
Permalink
Looks ok

--alex
Post by s***@oracle.com
Hi Alex,
Yes that is true, I had initially only done the one-liner {} because I
was unsure about the cases above :-)
Webrev: http://cr.openjdk.java.net/~jcbeyler/8214417/webrev.01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8214417
This was tested on my dev machine for all changed tests.
Thanks!
Jc
Ok, guys, you win :)
Looks like only 1-line initializers are fixed, so now we have mixed
ClearFieldAccessWatch/clrfldw001/clrfldw001.cpp (lines 52-53 are not
NULL },
51     { "nsk/jvmti/ClearFieldAccessWatch/clrfldw001", "fld1", "I", 1,
NULL },
52     {"nsk/jvmti/ClearFieldAccessWatch/clrfldw001", "fld2",
53         "Lnsk/jvmti/ClearFieldAccessWatch/clrfldw001a;", 0, NULL},
54     { "nsk/jvmti/ClearFieldAccessWatch/clrfldw001a", "fld3",
"[I", 0,
NULL },
The same in ClearFieldModificationWatch/clrfmodw001/clrfmodw001.cpp
51     { "nsk/jvmti/ClearFieldModificationWatch/clrfmodw001", "fld1",
"I", 1, NULL },
52     {"nsk/jvmti/ClearFieldModificationWatch/clrfmodw001", "fld2",
53         "Lnsk/jvmti/ClearFieldModificationWatch/clrfmodw001a;",
0, NULL},
54     { "nsk/jvmti/ClearFieldModificationWatch/clrfmodw001a", "fld3",
"[I", 0, NULL },
GetClassSignature/getclsig006/getclsig006.cpp (44-51)
43     { "getclsig006", "Lnsk/jvmti/GetClassSignature/getclsig006;",
"NULL" },
44     {"getclsig006b", "Lnsk/jvmti/GetClassSignature/getclsig006b;",
45         "<L:Ljava/lang/String;>Ljava/lang/Object;"},
46     {"getclsig006c", "Lnsk/jvmti/GetClassSignature/getclsig006c;",
47
 "<A:Ljava/lang/Object;B:Ljava/lang/Integer;>Ljava/lang/Object;"},
(and others files)
--alex
Post by Daniel D. Daugherty
I would also write code like what David shows... :-)
Dan
Post by David Holmes
I vote for the spaces around { and }
int foo() { return 1; }
static jvmtiEvent testEvents[] = { JVMTI_EVENT_THREAD_START,
JVMTI_EVENT_THREAD_END };
Cheers,
David
Post by s***@oracle.com
Hi Alex,
Post by Alex Menkov
Hi Serguei,
Post by s***@oracle.com
There are some implicit rules, like unification and consistency.
We want a space or new line after '{' and before '}'.
I believe this rule is about braces for code blocks (new line
after
Post by Daniel D. Daugherty
Post by David Holmes
Post by s***@oracle.com
Post by Alex Menkov
"{", "}" on a separate line), but this are array initializers.
Array initializers are blocs as well.
It make it a base for unification.
Post by Alex Menkov
And to me for 1-line array initializers spaces do not improve
readability.
I politely disagree, at least, they improve it for me. :)
We can do one simple test.
What suggestion would make the Jc's awk script simpler?
If yours then I'm out.
Otherwise, why does it make simpler for script but not for humans?
Also, we can wait for one more opinion.
Post by Alex Menkov
So to me this looks like the last item from "Whitespace" is
<cite>
Try not to change whitespace unless it improves readability or
consistency. (Different editors indent differently, and spurious
indentation changes will just make integrations more difficult.)
</cite>
It was Okay to break this rule when we decided to fix spacing over
all the tests.
Thanks,
Serguei
Post by Alex Menkov
--alex
Post by s***@oracle.com
Why this case needs to have an exception?
Thanks,
Serguei
Post by Alex Menkov
I don't see such rule (I suppose
https://wiki.openjdk.java.net/display/HotSpot/StyleGuide is
correct link?)
--alex
Post by s***@oracle.com
Post by Alex Menkov
Hi Jc,
In the JDK-8212771 review thread cleanup for "{}" was
requested
Post by Daniel D. Daugherty
Post by David Holmes
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
for statements like
test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/MethodBind/JvmtiTest/JvmtiTest.cpp,
Post by Daniel D. Daugherty
Post by David Holmes
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
+#define JVMTI_ERROR_CHECK(str,res) if (res !=
JVMTI_ERROR_NONE)
Post by Daniel D. Daugherty
Post by David Holmes
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
{ printf(str); printf("%d\n",res); return res;}
+#define JVMTI_ERROR_CHECK_EXPECTED_ERROR(str,res,err) if
(res
Post by Daniel D. Daugherty
Post by David Holmes
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
!= err) { printf(str); printf("unexpected error %d\n",res);
return res;}
I.e. something like ";}" -> "; }"
I don't think changes like
-static jvmtiEvent testEvents[] = {JVMTI_EVENT_THREAD_START,
JVMTI_EVENT_THREAD_END};
+static jvmtiEvent testEvents[] = { JVMTI_EVENT_THREAD_START,
JVMTI_EVENT_THREAD_END };
are required.
It is better to have it - rules are rules.
Thanks,
Serguei
Post by Alex Menkov
--alex
Post by JC Beyler
Hi all,
When working on a previous clean-up (JDK-8212771), I was
asked
Post by Daniel D. Daugherty
Post by David Holmes
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
Post by JC Beyler
to clean-up also spaces around {}.
Here is the first batch out of 2 to fix these cases. Let me
know what you think.
http://cr.openjdk.java.net/~jcbeyler/8214417/webrev.00/
Post by Daniel D. Daugherty
Post by David Holmes
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
Post by JC Beyler
Bug: https://bugs.openjdk.java.net/browse/JDK-8214417
Thanks for your reviews :-),
Jc
--
Thanks,
Jc
David Holmes
2018-11-30 00:44:44 UTC
Permalink
+1

This is burning a lot of cycles. :)

David
Post by Alex Menkov
Looks ok
--alex
Post by s***@oracle.com
Hi Alex,
Yes that is true, I had initially only done the one-liner {} because I
was unsure about the cases above :-)
Webrev: http://cr.openjdk.java.net/~jcbeyler/8214417/webrev.01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8214417
This was tested on my dev machine for all changed tests.
Thanks!
Jc
    Ok, guys, you win :)
    Looks like only 1-line initializers are fixed, so now we have mixed
    ClearFieldAccessWatch/clrfldw001/clrfldw001.cpp (lines 52-53 are not
    NULL },
    51     { "nsk/jvmti/ClearFieldAccessWatch/clrfldw001", "fld1",
"I", 1,
    NULL },
    52     {"nsk/jvmti/ClearFieldAccessWatch/clrfldw001", "fld2",
    53         "Lnsk/jvmti/ClearFieldAccessWatch/clrfldw001a;", 0, NULL},
    54     { "nsk/jvmti/ClearFieldAccessWatch/clrfldw001a", "fld3",
    "[I", 0,
    NULL },
    The same in ClearFieldModificationWatch/clrfmodw001/clrfmodw001.cpp
    51     { "nsk/jvmti/ClearFieldModificationWatch/clrfmodw001", "fld1",
    "I", 1, NULL },
    52     {"nsk/jvmti/ClearFieldModificationWatch/clrfmodw001", "fld2",
    53         "Lnsk/jvmti/ClearFieldModificationWatch/clrfmodw001a;",
    0, NULL},
    54     { "nsk/jvmti/ClearFieldModificationWatch/clrfmodw001a",
"fld3",
    "[I", 0, NULL },
    GetClassSignature/getclsig006/getclsig006.cpp (44-51)
    43     { "getclsig006", "Lnsk/jvmti/GetClassSignature/getclsig006;",
    "NULL" },
    44     {"getclsig006b", "Lnsk/jvmti/GetClassSignature/getclsig006b;",
    45         "<L:Ljava/lang/String;>Ljava/lang/Object;"},
    46     {"getclsig006c", "Lnsk/jvmti/GetClassSignature/getclsig006c;",
    47
 "<A:Ljava/lang/Object;B:Ljava/lang/Integer;>Ljava/lang/Object;"},
    (and others files)
    --alex
     > I would also write code like what David shows... :-)
     >
     > Dan
     >
     >
     >> I vote for the spaces around { and }
     >>
     >>
     >> int foo() { return 1; }
     >>
     >>
     >> static jvmtiEvent testEvents[] = { JVMTI_EVENT_THREAD_START,
     >> JVMTI_EVENT_THREAD_END };
     >>
     >> Cheers,
     >> David
     >>
     >>> Hi Alex,
     >>>
     >>>
     >>>> Hi Serguei,
     >>>>
     >>>>> There are some implicit rules, like unification and
consistency.
     >>>>> We want a space or new line after '{' and before '}'.
     >>>>
     >>>> I believe this rule is about braces for code blocks (new line
    after
     >>>> "{", "}" on a separate line), but this are array initializers.
     >>>
     >>> Array initializers are blocs as well.
     >>> It make it a base for unification.
     >>>
     >>>
     >>>> And to me for 1-line array initializers spaces do not improve
     >>>> readability.
     >>>
     >>> I politely disagree, at least, they improve it for me. :)
     >>>
     >>> We can do one simple test.
     >>> What suggestion would make the Jc's awk script simpler?
     >>> If yours then I'm out.
     >>> Otherwise, why does it make simpler for script but not for
humans?
     >>>
     >>> Also, we can wait for one more opinion.
     >>>
     >>>
     >>>> So to me this looks like the last item from "Whitespace" is
     >>>> <cite>
     >>>> Try not to change whitespace unless it improves readability or
     >>>> consistency. (Different editors indent differently, and
spurious
     >>>> indentation changes will just make integrations more
difficult.)
     >>>> </cite>
     >>>
     >>> It was Okay to break this rule when we decided to fix spacing
over
     >>> all the tests.
     >>>
     >>> Thanks,
     >>> Serguei
     >>>
     >>>
     >>>> --alex
     >>>>
     >>>>> Why this case needs to have an exception?
     >>>>>
     >>>>> Thanks,
     >>>>> Serguei
     >>>>>
     >>>>>
     >>>>>> I don't see such rule (I suppose
     >>>>>> https://wiki.openjdk.java.net/display/HotSpot/StyleGuide is
     >>>>>> correct link?)
     >>>>>>
     >>>>>> --alex
     >>>>>>
     >>>>>>>> Hi Jc,
     >>>>>>>>
     >>>>>>>> In the JDK-8212771 review thread cleanup for "{}" was
    requested
     >>>>>>>> for statements like
     >>>>>>>>
test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/MethodBind/JvmtiTest/JvmtiTest.cpp,
     >>>>>>>>
     >>>>>>>>
     >>>>>>>> +#define JVMTI_ERROR_CHECK(str,res) if (res !=
    JVMTI_ERROR_NONE)
     >>>>>>>> { printf(str); printf("%d\n",res); return res;}
     >>>>>>>>
     >>>>>>>> +#define JVMTI_ERROR_CHECK_EXPECTED_ERROR(str,res,err) if
    (res
     >>>>>>>> != err) { printf(str); printf("unexpected error %d\n",res);
     >>>>>>>> return res;}
     >>>>>>>>
     >>>>>>>> I.e. something like ";}" -> "; }"
     >>>>>>>>
     >>>>>>>>
     >>>>>>>> I don't think changes like
     >>>>>>>>
     >>>>>>>> -static jvmtiEvent testEvents[] =
{JVMTI_EVENT_THREAD_START,
     >>>>>>>> JVMTI_EVENT_THREAD_END};
     >>>>>>>> +static jvmtiEvent testEvents[] = {
JVMTI_EVENT_THREAD_START,
     >>>>>>>> JVMTI_EVENT_THREAD_END };
     >>>>>>>>
     >>>>>>>> are required.
     >>>>>>>
     >>>>>>> It is better to have it - rules are rules.
     >>>>>>>
     >>>>>>> Thanks,
     >>>>>>> Serguei
     >>>>>>>
     >>>>>>>>
     >>>>>>>> --alex
     >>>>>>>>
     >>>>>>>>
     >>>>>>>>> Hi all,
     >>>>>>>>>
     >>>>>>>>> When working on a previous clean-up (JDK-8212771), I was
    asked
     >>>>>>>>> to clean-up also spaces around {}.
     >>>>>>>>>
     >>>>>>>>> Here is the first batch out of 2 to fix these cases.
Let me
     >>>>>>>>> know what you think.
     >>>>>>>>>
    http://cr.openjdk.java.net/~jcbeyler/8214417/webrev.00/
     >>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8214417
     >>>>>>>>>
     >>>>>>>>> Thanks for your reviews :-),
     >>>>>>>>> Jc
     >>>>>>>
     >>>>>
     >>>
     >
s***@oracle.com
2018-11-30 02:45:53 UTC
Permalink
+1

Thanks,
Serguei
Post by David Holmes
+1
This is burning a lot of cycles. :)
David
Post by Alex Menkov
Looks ok
--alex
Post by s***@oracle.com
Hi Alex,
Yes that is true, I had initially only done the one-liner {} because
I was unsure about the cases above :-)
Webrev: http://cr.openjdk.java.net/~jcbeyler/8214417/webrev.01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8214417
This was tested on my dev machine for all changed tests.
Thanks!
Jc
On Thu, Nov 29, 2018 at 2:26 PM Alex Menkov
    Ok, guys, you win :)
    Looks like only 1-line initializers are fixed, so now we have mixed
    ClearFieldAccessWatch/clrfldw001/clrfldw001.cpp (lines 52-53 are not
    NULL },
    51     { "nsk/jvmti/ClearFieldAccessWatch/clrfldw001", "fld1",
"I", 1,
    NULL },
    52     {"nsk/jvmti/ClearFieldAccessWatch/clrfldw001", "fld2",
    53  "Lnsk/jvmti/ClearFieldAccessWatch/clrfldw001a;", 0, NULL},
    54     { "nsk/jvmti/ClearFieldAccessWatch/clrfldw001a", "fld3",
    "[I", 0,
    NULL },
    The same in ClearFieldModificationWatch/clrfmodw001/clrfmodw001.cpp
    51     { "nsk/jvmti/ClearFieldModificationWatch/clrfmodw001", "fld1",
    "I", 1, NULL },
    52  {"nsk/jvmti/ClearFieldModificationWatch/clrfmodw001", "fld2",
    53  "Lnsk/jvmti/ClearFieldModificationWatch/clrfmodw001a;",
    0, NULL},
    54     { "nsk/jvmti/ClearFieldModificationWatch/clrfmodw001a",
"fld3",
    "[I", 0, NULL },
    GetClassSignature/getclsig006/getclsig006.cpp (44-51)
    43     { "getclsig006",
"Lnsk/jvmti/GetClassSignature/getclsig006;",
    "NULL" },
    44     {"getclsig006b",
"Lnsk/jvmti/GetClassSignature/getclsig006b;",
    45  "<L:Ljava/lang/String;>Ljava/lang/Object;"},
    46     {"getclsig006c",
"Lnsk/jvmti/GetClassSignature/getclsig006c;",
    47
 "<A:Ljava/lang/Object;B:Ljava/lang/Integer;>Ljava/lang/Object;"},
    (and others files)
    --alex
     > I would also write code like what David shows... :-)
     >
     > Dan
     >
     >
     >> I vote for the spaces around { and }
     >>
     >>
     >> int foo() { return 1; }
     >>
     >>
     >> static jvmtiEvent testEvents[] = { JVMTI_EVENT_THREAD_START,
     >> JVMTI_EVENT_THREAD_END };
     >>
     >> Cheers,
     >> David
     >>
     >>> Hi Alex,
     >>>
     >>>
     >>>> Hi Serguei,
     >>>>
     >>>>> There are some implicit rules, like unification and
consistency.
     >>>>> We want a space or new line after '{' and before '}'.
     >>>>
     >>>> I believe this rule is about braces for code blocks (new line
    after
     >>>> "{", "}" on a separate line), but this are array initializers.
     >>>
     >>> Array initializers are blocs as well.
     >>> It make it a base for unification.
     >>>
     >>>
     >>>> And to me for 1-line array initializers spaces do not improve
     >>>> readability.
     >>>
     >>> I politely disagree, at least, they improve it for me. :)
     >>>
     >>> We can do one simple test.
     >>> What suggestion would make the Jc's awk script simpler?
     >>> If yours then I'm out.
     >>> Otherwise, why does it make simpler for script but not for
humans?
     >>>
     >>> Also, we can wait for one more opinion.
     >>>
     >>>
     >>>> So to me this looks like the last item from "Whitespace" is
     >>>> <cite>
     >>>> Try not to change whitespace unless it improves readability or
     >>>> consistency. (Different editors indent differently, and
spurious
     >>>> indentation changes will just make integrations more
difficult.)
     >>>> </cite>
     >>>
     >>> It was Okay to break this rule when we decided to fix
spacing over
     >>> all the tests.
     >>>
     >>> Thanks,
     >>> Serguei
     >>>
     >>>
     >>>> --alex
     >>>>
     >>>>> Why this case needs to have an exception?
     >>>>>
     >>>>> Thanks,
     >>>>> Serguei
     >>>>>
     >>>>>
     >>>>>> I don't see such rule (I suppose
     >>>>>> https://wiki.openjdk.java.net/display/HotSpot/StyleGuide is
     >>>>>> correct link?)
     >>>>>>
     >>>>>> --alex
     >>>>>>
     >>>>>>>> Hi Jc,
     >>>>>>>>
     >>>>>>>> In the JDK-8212771 review thread cleanup for "{}" was
    requested
     >>>>>>>> for statements like
     >>>>>>>>
test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/MethodBind/JvmtiTest/JvmtiTest.cpp,
     >>>>>>>>
     >>>>>>>>
     >>>>>>>> +#define JVMTI_ERROR_CHECK(str,res) if (res !=
    JVMTI_ERROR_NONE)
     >>>>>>>> { printf(str); printf("%d\n",res); return res;}
     >>>>>>>>
     >>>>>>>> +#define JVMTI_ERROR_CHECK_EXPECTED_ERROR(str,res,err) if
    (res
     >>>>>>>> != err) { printf(str); printf("unexpected error %d\n",res);
     >>>>>>>> return res;}
     >>>>>>>>
     >>>>>>>> I.e. something like ";}" -> "; }"
     >>>>>>>>
     >>>>>>>>
     >>>>>>>> I don't think changes like
     >>>>>>>>
     >>>>>>>> -static jvmtiEvent testEvents[] =
{JVMTI_EVENT_THREAD_START,
     >>>>>>>> JVMTI_EVENT_THREAD_END};
     >>>>>>>> +static jvmtiEvent testEvents[] = {
JVMTI_EVENT_THREAD_START,
     >>>>>>>> JVMTI_EVENT_THREAD_END };
     >>>>>>>>
     >>>>>>>> are required.
     >>>>>>>
     >>>>>>> It is better to have it - rules are rules.
     >>>>>>>
     >>>>>>> Thanks,
     >>>>>>> Serguei
     >>>>>>>
     >>>>>>>>
     >>>>>>>> --alex
     >>>>>>>>
     >>>>>>>>
     >>>>>>>>> Hi all,
     >>>>>>>>>
     >>>>>>>>> When working on a previous clean-up (JDK-8212771), I was
    asked
     >>>>>>>>> to clean-up also spaces around {}.
     >>>>>>>>>
     >>>>>>>>> Here is the first batch out of 2 to fix these cases.
Let me
     >>>>>>>>> know what you think.
     >>>>>>>>>
    http://cr.openjdk.java.net/~jcbeyler/8214417/webrev.00/
     >>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8214417
     >>>>>>>>>
     >>>>>>>>> Thanks for your reviews :-),
     >>>>>>>>> Jc
     >>>>>>>
     >>>>>
     >>>
     >
--
JC Beyler
2018-11-30 02:59:18 UTC
Permalink
Thanks all, and pushed :)
Jc
Post by s***@oracle.com
+1
Thanks,
Serguei
Post by David Holmes
+1
This is burning a lot of cycles. :)
David
Post by Alex Menkov
Looks ok
--alex
Post by s***@oracle.com
Hi Alex,
Yes that is true, I had initially only done the one-liner {} because
I was unsure about the cases above :-)
Webrev: http://cr.openjdk.java.net/~jcbeyler/8214417/webrev.01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8214417
This was tested on my dev machine for all changed tests.
Thanks!
Jc
On Thu, Nov 29, 2018 at 2:26 PM Alex Menkov
Ok, guys, you win :)
Looks like only 1-line initializers are fixed, so now we have mixed
ClearFieldAccessWatch/clrfldw001/clrfldw001.cpp (lines 52-53 are not
NULL },
51 { "nsk/jvmti/ClearFieldAccessWatch/clrfldw001", "fld1", "I", 1,
NULL },
52 {"nsk/jvmti/ClearFieldAccessWatch/clrfldw001", "fld2",
53 "Lnsk/jvmti/ClearFieldAccessWatch/clrfldw001a;", 0, NULL},
54 { "nsk/jvmti/ClearFieldAccessWatch/clrfldw001a", "fld3", "[I", 0,
NULL },
The same in ClearFieldModificationWatch/clrfmodw001/clrfmodw001.cpp
51 { "nsk/jvmti/ClearFieldModificationWatch/clrfmodw001", "fld1",
"I", 1, NULL },
52 {"nsk/jvmti/ClearFieldModificationWatch/clrfmodw001", "fld2",
53 "Lnsk/jvmti/ClearFieldModificationWatch/clrfmodw001a;",
0, NULL},
54 { "nsk/jvmti/ClearFieldModificationWatch/clrfmodw001a", "fld3",
"[I", 0, NULL },
GetClassSignature/getclsig006/getclsig006.cpp (44-51)
43 { "getclsig006",
"Lnsk/jvmti/GetClassSignature/getclsig006;",
"NULL" },
44 {"getclsig006b",
"Lnsk/jvmti/GetClassSignature/getclsig006b;",
45 "<L:Ljava/lang/String;>Ljava/lang/Object;"},
46 {"getclsig006c",
"Lnsk/jvmti/GetClassSignature/getclsig006c;",
47
"<A:Ljava/lang/Object;B:Ljava/lang/Integer;>Ljava/lang/Object;"},
(and others files)
--alex
Post by Daniel D. Daugherty
I would also write code like what David shows... :-)
Dan
Post by David Holmes
I vote for the spaces around { and }
int foo() { return 1; }
static jvmtiEvent testEvents[] = { JVMTI_EVENT_THREAD_START,
JVMTI_EVENT_THREAD_END };
Cheers,
David
Post by s***@oracle.com
Hi Alex,
Post by Alex Menkov
Hi Serguei,
Post by s***@oracle.com
There are some implicit rules, like unification and
consistency.
Post by Daniel D. Daugherty
Post by David Holmes
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
We want a space or new line after '{' and before '}'.
I believe this rule is about braces for code blocks (new line
after
Post by Daniel D. Daugherty
Post by David Holmes
Post by s***@oracle.com
Post by Alex Menkov
"{", "}" on a separate line), but this are array
initializers.
Post by Daniel D. Daugherty
Post by David Holmes
Post by s***@oracle.com
Array initializers are blocs as well.
It make it a base for unification.
Post by Alex Menkov
And to me for 1-line array initializers spaces do not improve
readability.
I politely disagree, at least, they improve it for me. :)
We can do one simple test.
What suggestion would make the Jc's awk script simpler?
If yours then I'm out.
Otherwise, why does it make simpler for script but not for
humans?
Post by Daniel D. Daugherty
Post by David Holmes
Post by s***@oracle.com
Also, we can wait for one more opinion.
Post by Alex Menkov
So to me this looks like the last item from "Whitespace" is
<cite>
Try not to change whitespace unless it improves
readability or
Post by Daniel D. Daugherty
Post by David Holmes
Post by s***@oracle.com
Post by Alex Menkov
consistency. (Different editors indent differently, and
spurious
Post by Daniel D. Daugherty
Post by David Holmes
Post by s***@oracle.com
Post by Alex Menkov
indentation changes will just make integrations more
difficult.)
Post by Daniel D. Daugherty
Post by David Holmes
Post by s***@oracle.com
Post by Alex Menkov
</cite>
It was Okay to break this rule when we decided to fix
spacing over
Post by Daniel D. Daugherty
Post by David Holmes
Post by s***@oracle.com
all the tests.
Thanks,
Serguei
Post by Alex Menkov
--alex
Post by s***@oracle.com
Why this case needs to have an exception?
Thanks,
Serguei
Post by Alex Menkov
I don't see such rule (I suppose
https://wiki.openjdk.java.net/display/HotSpot/StyleGuide
is
Post by David Holmes
Post by Alex Menkov
Post by s***@oracle.com
Post by Daniel D. Daugherty
Post by David Holmes
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
correct link?)
--alex
Post by s***@oracle.com
Post by Alex Menkov
Hi Jc,
In the JDK-8212771 review thread cleanup for "{}" was
requested
Post by Daniel D. Daugherty
Post by David Holmes
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
for statements like
test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/MethodBind/JvmtiTest/JvmtiTest.cpp,
Post by David Holmes
Post by Alex Menkov
Post by s***@oracle.com
Post by Daniel D. Daugherty
Post by David Holmes
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
+#define JVMTI_ERROR_CHECK(str,res) if (res !=
JVMTI_ERROR_NONE)
Post by Daniel D. Daugherty
Post by David Holmes
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
{ printf(str); printf("%d\n",res); return res;}
+#define JVMTI_ERROR_CHECK_EXPECTED_ERROR(str,res,err) if
(res
Post by Daniel D. Daugherty
Post by David Holmes
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
!= err) { printf(str); printf("unexpected error
%d\n",res);
Post by Daniel D. Daugherty
Post by David Holmes
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
return res;}
I.e. something like ";}" -> "; }"
I don't think changes like
-static jvmtiEvent testEvents[] =
{JVMTI_EVENT_THREAD_START,
Post by Daniel D. Daugherty
Post by David Holmes
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
JVMTI_EVENT_THREAD_END};
+static jvmtiEvent testEvents[] = {
JVMTI_EVENT_THREAD_START,
Post by Daniel D. Daugherty
Post by David Holmes
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
JVMTI_EVENT_THREAD_END };
are required.
It is better to have it - rules are rules.
Thanks,
Serguei
Post by Alex Menkov
--alex
Post by JC Beyler
Hi all,
When working on a previous clean-up (JDK-8212771), I was
asked
Post by Daniel D. Daugherty
Post by David Holmes
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
Post by JC Beyler
to clean-up also spaces around {}.
Here is the first batch out of 2 to fix these cases.
Let me
Post by Daniel D. Daugherty
Post by David Holmes
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
Post by JC Beyler
know what you think.
http://cr.openjdk.java.net/~jcbeyler/8214417/webrev.00/
Post by Daniel D. Daugherty
Post by David Holmes
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
Post by s***@oracle.com
Post by Alex Menkov
Post by JC Beyler
Bug: https://bugs.openjdk.java.net/browse/JDK-8214417
Thanks for your reviews :-),
Jc
--
Thanks,
Jc
--
Thanks,
Jc
s***@oracle.com
2018-11-28 23:48:08 UTC
Permalink
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Hi Jc,<br>
<br>
LGTM.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 11/28/18 11:20, JC Beyler wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAF9BGByvn47YyD+9oHbyoshWexiOK=XGTpX5-=O=***@mail.gmail.com">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">Hi all,
<div><br>
</div>
<div>When working on a previous clean-up (JDK-8212771), I
was asked to clean-up also spaces around {}.</div>
<div><br>
</div>
<div>Here is the first batch out of 2 to fix these cases.
Let me know what you think.</div>
<div><br>
</div>
<div>Webrev: <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8214417/webrev.00/"
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8214417/webrev.00/</a></div>
<div>Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8214417"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8214417</a></div>
<div>
<div><br>
</div>
<div dir="ltr" class="gmail_signature">
<div dir="ltr">Thanks for your reviews :-),
<div>Jc</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
</body>
</html>
Loading...