Am 02.12.2011 16:27, schrieb Brandon Passanisi:
Hi Sebastian. I'm not sure if you had seen the e-mail from Stuart
Marks regarding this, but Stuart was able to find more instances of
the similar block of "fallthrough" code. I can volunteer to apply
your upcoming change to FilePermission to the other files if you
wish. Or, you can try applying the change to the other files, but if
you don't have time I don't mind doing it. Here's the section of
Stuart's e-mail on this topic:
-------------------------------------------------------------------------------------
(Incidentally, this is the third time I've reviewed code today that
looks exactly like this. The other cases are in
java.io.FilePermission
and java.util.PropertyPermission. They each have the /*FALLTHROUGH*/
into a set of cases that do nothing but break; and they have similar
("ackbarfaccept") comments. It would be nice if these chunks of code
could be unified, but they differ in a number of fiddly details.)
(The string "ackbarfaccept" occurs in the following files:
1. java/io/FilePermission.java
2. java/net/SocketPermission.java
3. java/util/PropertyPermission.java
4. javax/management/MBeanPermission.java
5. javax/security/auth/kerberos/ServicePermission.java
Hmmm.)
-------------------------------------------------------------------------------------
Thanks.
Hi Brandon,
Hi Stuart,
i had a look at all those classes and it seems to be the same algorithm.
In an normal project (not the jdk) i would suggest to completely
refactor it and make some code de-duplication cleanup. But if i thinks
on projects like jigsaw this can easily get a real problem. What do you
think, should we try to cleanup them all. Or should we try to make some
de-duplication/code-reuse refactoring.
-- Sebastian
On 12/1/2011 10:18 PM, Sebastian Sickelmann wrote:
Hi Brandon,
i will try to work out a fix for both and cc the review request to you.
-- Sebastian
Am 01.12.2011 23:54, schrieb Brandon Passanisi:
Hi Sebastian. I was speaking with Stuart Marks earlier today and he
mentioned that the "fallthrough" code in FilePermission.java also
exists in java.util.PropertyPermission.java. Maybe the code author
had done some copy/paste when working on these files. Stuart had
said that you might be planning on doing some work on this after the
warnings cleanup work. If/when you do this work, maybe you can let
me know so that I can sync the same changes you apply to
FilePermission.java to PropertyPermission.java? Or, maybe you can
apply the same changes yourself to PropertyPermission.java?
Thanks.
On 12/1/2011 8:27 AM, Sebastian Sickelmann wrote:
Am 01.12.2011 10:16, schrieb Stuart Marks:
Hi Sebastian,
Thanks for submitting this patch! I've filed bug 7116890 to cover
this set of changes.
Regarding webrev, it probably does get confused by merges in the
history. By default it tries to show differences between the tip
of the target repository and tip + uncommitted changes in your
local repository. This isn't always the right thing. One thing you
might try is to use the -r option to specify a particular revision
in the history against which to generate the diffs. But, having a
few extra files isn't a big deal.
On to your changes.
Most of them are really good and are exactly the kind of simple
change we're looking for, as I posted a little while ago. [1]
The ExpiringCache.java case is an interesting one. I think it's
possible to add a static serialVersionUID field within the
anonymous class. It's hard to run serialver over an anonymous
class (actually it might be possible to run the checksum algorithm
on the loaded class) but if in practice it's never serialized, who
cares what the "correct" value is? You could just use a value of 0L.
An alternative would also be to use @SuppressWarnings("serial").
Since the constructor is so short, you could just put it on the
constructor.
Actually I prefer @SuppressWarnings, since adding serialVersionUID
adds to the system's footprint. If it's never used, we shouldn't
bother defining it.
Given these alternatives, it's not necessary to create a static
CacheHashMapImpl class.
Regarding the FilePermission.java change, yes, I see that the
skipBeforePreviousComma() change is problematic. I puzzled over it
for a while too. I wish I had sent out the Coding Guideline [1]
earlier; it might have saved you an hour. :-) Sorry about that.
A cleanup/rewrite of this code would indeed be better done
separate from Warnings Cleanup Day. But I think there might be a
simple way to get rid of the warning without doing a cleanup or
refactoring. The warning message in question is at line 535 of the
original code, about falling through to the next case. But the
next case doesn't actually do anything but break. Could we just
change the /*FALLTHROUGH*/ comment to a break statement? I think
that would fix the warning without changing the logic at all.
s'marks
[1]
http://mail.openjdk.java.net/pipermail/jdk8-dev/2011-December/000380.html
On 11/30/11 8:33 PM, Sebastian Sickelmann wrote:
Hi,
i have a webrev[0] that contains some warning cleanup for java.io
It is based on rev 7795c41ed54c
http://hg.openjdk.java.net/jdk8/tl/jdk/
Some comments to the changes:
ExpiringCache.java: Changed anonymous inner class to inner class
with the
intention to put serialversion inside of it. But serialver
doesn't want to give
my the serialver. I also think that ExpiringCache is not
serializable but the
warning was clear: the anonymous inner class is seriallizable and
has no
explicit serialversionuid.
FilePermission.java: I have starred at the code between line 453
and 547 for
over an hour, because i thought that there is a bug within the
expression "i >=
matchlen" in line 530 and the both "i != -1" in lines 457 and
461. But there is
no bug. But i wanted to left this code slightly more readable. I
introduced the
method skipBeforePreviousComma to make it possible to work-around
the
fallthought warning with an return statement. This code-change
need's some more
review attention. Maybe we should split this up for another
cleanup. I think
the whole method needs some rewrite.
Some classes had no change at all. Maybe webrev created them
because there
where changes in my history/branches. There were some patches
from alan i saw
to late. Maybe webrev is confused of the multiple merges.
Can someone please create a CR for this and
[0]
http://dl.dropbox.com/u/43692695/oss-patches/openjdk8/Warning_Cleanup_Java_io/webrev0_based_on_7795c41ed54c/index.html
Thanks for the good feedback.
I splitted my change to the trivial part and will start discussion
of FilePermission change on core-libs-dev after the cleanup event.
I created a new webrev with the suggested changes here[2]
[2]
http://dl.dropbox.com/u/43692695/oss-patches/openjdk8/Warning_Cleanup_Java_io/CR7116890_0/index.html
-- Sebastian
--
Oracle <http://www.oracle.com>
Brandon Passanisi | Principle Member of Technical Staff
Oracle Java Standards Conformance
Green Oracle <http://www.oracle.com/commitment> Oracle is committed
to developing practices and products that help protect the environment
