Hi Martijn,
Thanks for this contribution. I'm finally getting back to it. (Also, Kurchi,
Remi, thanks for taking a look as well.)
The java.util patches look good and are almost ready to go in. The only issue
is how to format the Contributed-by line in the changeset comment. What I have
so far for the comment is:
7176907: additional warnings cleanup in java.util, java.util.regexp,
java.util.zip
Reviewed-by: forax, khazra, smarks
Contributed-by: ???
The contributed-by line takes one or more email addresses or email/name pairs.
For an earlier contribution from LJC, see here [1]. This isn't a terribly big
deal but I want to make sure that credit goes where it's due. Can you tell me
what I should put for the contributed-by line?
The warnings in java.text have already been fixed by Deepak Bhole's changeset
[2]. Not a problem, this took two minutes to figure out.
There were a couple questions from earlier in the thread.
On the discussion of when the compiler issues switch fallthrough warnings, from
what I can tell, the compiler issues a warning whenever there is actual code in
a case that doesn't end with break, continue, return, or throw. This seems
independent of whether what follows is another case or the 'default' case. If
there are several case clauses together with no intervening code, this isn't
considered a fallthrough and thus there is no warning. This make sense, as
sometimes you want several different cases to be handled by the same code. For
example,
switch (ch) {
case 'a':
// no warning here
case 'b':
someActualWork();
break;
// ...
}
Regarding whether there is a style checker for indentation and spacing, I'm not
aware of a good one that I can recommend. We generally adhere to the (very old)
Java Coding Conventions [3]. I think most people just deal with style issues
manually by hand and by eye; I know I do. We do run jcheck [4] on every
incoming changeset, but the only things it checks in files' contents are for
trailing whitespace, and CR (as opposed to LF) and TAB characters.
s'marks
[1] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/445ada5e6b4a
[2] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e309917fb9af
[3] http://www.oracle.com/technetwork/java/codeconv-138413.html
[4] http://hg.openjdk.java.net/code-tools/jcheck
On 6/27/12 9:09 AM, Martijn Verburg wrote:
Hi all,
We've been working on improving the workflow for our patch review
system and so the new locations for the patches are at:
https://raw.github.com/AdoptOpenJDK/PatchReview/master/5-submitted/javac_warnings/core_java_text.patch
https://raw.github.com/AdoptOpenJDK/PatchReview/master/5-submitted/javac_warnings/core_java_util.patch
Cheers,
Martijn
On 27 June 2012 08:58, Martijn Verburg<martijnverb...@gmail.com> wrote:
Hi Kurchi,
Thanks for updating this. This looks good to me. I guess Stuart will be
sponsoring the patch.
For his sins he's kindly offered to do so yes :-)
There are a couple of other switch statements in
src/share/classes/java/util/regex/Pattern.java.
which are not throwing fallthrough warnings (in Netbeans at least),
although there is fallthrough happening from one case to another. From what
I notice, only if there is a break statement
missing before the "default" case, does Netbeans throw some warning. Is the
fallthrough warning technically
supposed to be thrown only when the logic falls through to the default case
then?
That's my understanding with the javac compiler yes. I think it was the balance
that the javac folk thought was a sensible compromise in terms of not
generating
too many false positive warnings.
Cheers,
Martijn
