please review 7117612: warnings fixes in java.lang (original) (raw)

Omair Majid omajid at redhat.com
Thu Dec 8 16:25:21 UTC 2011

On 12/08/2011 01:22 AM, Stuart Marks wrote:

On 12/7/11 3:13 PM, Omair Majid wrote:

On 12/07/2011 05:43 AM, Alan Bateman wrote:

I looked through the latest webrev. In EnumConstantNotPresentException.java then the @SuppressWarnings("rawtypes") should probably have a comment to explain why it is needed. In ThreadLocal.get then it's a pity that an additional local is needed to increase the size of the method. Otherwise the changes look okay to me.

Updated webrev at: http://cr.openjdk.java.net/~omajid/webrevs/warnings-day-2011/04/ Is there something I should do to address the extra local, or is it good to go? Hi Omair, Everything looks good to me. I think Alan was lamenting that adding the local variable for the sole purpose of adding the @SuppressWarnings annotation makes the method appear longer and more complex. The alternative is to put @SuppressWarnings on the entire method, which we've consistently frowned on, so I don't see the need to change anything.

Thanks for the clarification. Since this is the JDK code, I wasnt sure if this is something that might lead to a problem.

By the way, I've also run this changeset through our internal multi-platform build and test system, and everything works fine (with the exception of intermittent failures unrelated to this change).

That's great to know. I feel a little more confident about this changeset now.

You have commit rights, don't you? I'd say it's OK to proceed with the push. Or, if you prefer, Alan should be online in just a couple hours, and I'm sure he can give the final go-ahead.

Yes, I do have commit rights and I would be more than happy to push the changeset. One quick question, if you dont mind. Does this comment look fine?

7117612: Miscellaneous warnings in java.lang Reviewed-by: smarks, dholmes, alanb

Shall I add Joe Darcy to the list of reviewers too? He did make a comment on AutoClosable, but I am not sure if he was a 'reviewer'.

Thanks for working on this!

Glad I could help out.

Cheers, Omair

More information about the core-libs-dev mailing list