Bug 7176907 - Patches for javac warnings cleanup (text and util) from Adopt OpenJDK

[Stuart Marks]

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<martijnverburg at 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