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

Stuart Marks stuart.marks at oracle.com
Fri Jun 29 20:40:37 UTC 2012

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/javacwarnings/corejavatext.patch https://raw.github.com/AdoptOpenJDK/PatchReview/master/5-submitted/javacwarnings/corejavautil.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

More information about the core-libs-dev mailing list