Thanks for cleaning up those spaces Dan. The changes look fine.
Sorry for the extra trouble!
- Kurchi
On 8/28/12 10:22 PM, Dan Xu wrote:
It is funny. :) I have searched all source codes under jdk and removed
spaces for the similar cases.
Please review the new version of change at,
http://cr.openjdk.java.net/~dxu/7193406/webrev.03/.
Thanks for your comment!
-Dan
On 08/28/2012 05:32 PM, Kurchi Hazra wrote:
Irony of the day - those changes were done by me!
(http://cr.openjdk.java.net/~khazra/7157893/webrev.02/) :D
They were probably a mistake/oversight. I guess the better way is
without those extra spaces. See
http://docs.oracle.com/javase/tutorial/java/javaOO/annotations.html.
If you have time, maybe you can remove those spaces I put in as a
part of this CR.
Thanks!
Kurchi
On 8/28/2012 5:23 PM, Dan Xu wrote:
I also thought the space was not needed. But when I made the
changes, I found that many similar codes had the space when two or
more warning types need to be suppressed. For example,
java/util/Collections.java, java/util/Arrays.java,
java/util/ComparableTimSort.java, and etc. If only one warning type
needs to be suppressed, I don't see the space in our codes. Thanks!
-Dan
On 08/28/2012 05:08 PM, Kurchi Hazra wrote:
I don't think you need the space before "unchecked" and the one
after "rawtypes" in lines 128 and 147 in
http://cr.openjdk.java.net/~dxu/7193406/webrev.02/src/share/classes/java/util/PropertyResourceBundle.java.sdiff.html.
- Kurchi
On 8/28/2012 4:57 PM, Dan Xu wrote:
Thanks for all your good suggestions!
I have updated my changes, which revoke changes to makefiles and
put @SuppressWarnings outside methods instead of introducing local
variables for small methods.
The webrev is at
http://cr.openjdk.java.net/~dxu/7193406/webrev.02/. Thanks!
-Dan
On 08/27/2012 04:33 PM, Stuart Marks wrote:
On 8/27/12 3:55 AM, Doug Lea wrote:
The underlying issue is that code size is one of the criteria
that JITs use to decide to compile/inline etc. So long as they do
so, there will be cases here and there where it critically
important to keep sizes small in bottleneck code. Not many,
but still enough for me to object to efforts that might
blindly increase code size for the sake of warnings cleanup.
I'm pleased that warnings cleanup has attracted this much
attention. :-)
I was wondering where rule about @SuppressWarnings and local
variables originally came from, and I tracked this back to
Effective Java, Item 24, which says (as part of a fairly long
discussion)
If you find yourself using the SuppressWarnings annotation
on a method or constructor that's more than one line long,
you may be able to move it onto a local variable declaration.
You may have to declare a new local variable, but it's worth it.
Aha! So it's all Josh Bloch's fault! :-)
In the warnings cleanup thus far, and in Dan's webrev, we've
followed this rule fairly strictly. But since we seem to have
evidence that this change isn't worth it, we should consider
relaxing the rule for performance-critical code. How about adding
a local variable for @SuppressWarnings only if the method is,
say, longer than ten lines? (Or some other suitable threshold.)
For short methods the annotation should be placed on the method
itself.
The risk of suppressing other warnings inadvertently is pretty
small in a short method, and short methods are the ones most
likely to be impacted by the addition of a local variable
affecting compilation/inlining decisions. Right?
(Also, while I've recommended that people follow the local
variable rule fairly strictly, I think it tends to garbage up
short methods. So I wouldn't mind seeing the rule relaxed on
readability grounds as well.)
s'marks
--
-Kurchi
