Re: Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io
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
Re: [7u8] Request for approval: 7160252: (prefs) NodeAddedEvent was not delivered when new node add when new Node
Sorry for the delay. Approved. Cheers, Edvard Ps. Please start new threads instead of replying to an old approval request. On 08/28/2012 02:22 AM, Kurchi Hazra wrote: This is a request for approval to backport the fix for 7160252 from 8 to 7u8. Bug:http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7160252 Webrev:http://cr.openjdk.java.net/~khazra/7160252/7u8/webrev.00/ This had been reviewed by Alan Bateman and Chris Hegarty. [1] This fix has been pushed into jdk8 [2]. Thanks, Kurchi [1]http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-July/010823.html [2]http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e9461aeff91f
Re: Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io
On 08/29/2012 08:33 AM, Kurchi Subhra Hazra wrote: 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 Hi Dan, In PreHashedMap, the code should be Map.Entry?,? that = (Map.Entry?,?)ob; so the @SuppressWarnings is not needed anymore. And in java.util.Arrays, asList() should not be annotated with @SuppressWarnings because it is already annotated with SafeVarargs, so the compiler should not report the varargs warning. I have CC to compiler-dev to be sure that this is a compiler error. cheers, Rémi
Re: RFR [JDK8]: 7169894: JAXP Plugability Layer: using service loader
Hi Joe,
I would urge you to reconsider errors using SL, since SL is being explicitly
called out as part of the specification.
You can make any such SL-related errors more meaningful (yes, i want the stack
trace telling what bits of SL code were called!) and remove any potential for
CCEs, due to linkage errors [*], by passing the SCE instance to the constructor
of DatatypeConfigurationException or the FactoryConfigurationError variants e.g.
244 } catch (ServiceConfigurationError e) {
245 throw new DatatypeConfigurationException(Provider +
className + could not be loaded or instantiated using
java.util.ServiceLoader, e);
--
For the SchemaFactoryFinder there is now a potentially subtle difference in
what CL is used to load a class by SL and by
SchemaFactoryFinder.createInstance. Previously the same CL was always used
whatever the mechanism was used to identify the class name. Errors related to
class loaders can be extremely hard to track down hence the conservative
position.
--
I will say no more on the above :-)
Paul.
[*] If a developer can declare a service provider then such linkage errors can
potentially occur.
On Aug 28, 2012, at 7:46 PM, Joe Wang huizhe.w...@oracle.com wrote:
On 8/28/2012 1:19 AM, Paul Sandoz wrote:
Hi Joe,
On Aug 28, 2012, at 7:35 AM, Joe Wang huizhe.w...@oracle.com wrote:
--
datatype/FactoryFinder.java:
244 } catch (ServiceConfigurationError e) {
245 throw new DatatypeConfigurationException(e.getMessage(),
e.getCause());
You are munging the message of the exception and it's cause. Perhaps it
would be better just to pass along the SCE as the cause, that way it is
better identified that SL is being used when an error occurs.
None of the ConfigureError classes in other packages accept Error or
Throwable as did DataType (and this one is an Exception!)
So instead of making changes to the ConfigureError classes, I wrapped the
ServiceConfigurationError in jaxp configuration errors, and in this case
(Datatype), a datatype configuration exception
It should be very rare to get a ServiceConfigurationError that would
indicate a serious error in a jar configuration, basically, a non-usable
implementation. So I think we don't have to stick with the
ServiceConfigurationError.
If there is an SCE it is hard for the developer to trace. Was SL used or
not? How would a developer know?
The error message is clear enough - the following for example shows the error
with or without the cause. There has been no error being reported back ever
since the start of jaxp, as you noticed, it always falls back to the default
imp, and we never heard any complaint since implementing jaxp is just not a
routing work. Adding the cause, we would have to change the signatures of
the FactoryConfigurationError, sth. we've trying to avoid in this patch.
javax.xml.parsers.FactoryConfigurationError:
javax.xml.parsers.DocumentBuilderFactory: Provider
test.factoryfinder2.MyDocumentBuilderFactoryXX not found
at
javax.xml.parsers.FactoryFinder.findServiceProvider(FactoryFinder.java:300)
at javax.xml.parsers.FactoryFinder.find(FactoryFinder.java:258)
at
javax.xml.parsers.DocumentBuilderFactory.newInstance(DocumentBuilderFactory.java:142)
at common.Bug7169894Test.testDOMFactory(Bug7169894Test.java:85)
Caused by: java.util.ServiceConfigurationError:
javax.xml.parsers.DocumentBuilderFactory: Provider
test.factoryfinder2.MyDocumentBuilderFactoryXX not found
at java.util.ServiceLoader.fail(ServiceLoader.java:214)
at java.util.ServiceLoader.access$400(ServiceLoader.java:164)
at java.util.ServiceLoader$LazyIterator.next(ServiceLoader.java:350)
at java.util.ServiceLoader$1.next(ServiceLoader.java:421)
at javax.xml.parsers.FactoryFinder$1.run(FactoryFinder.java:286)
at java.security.AccessController.doPrivileged(Native Method)
at
javax.xml.parsers.FactoryFinder.findServiceProvider(FactoryFinder.java:283)
The cast from Throwable to Exception should be avoided, here is the code in
SL:
public S next() {
if (!hasNext()) {
throw new NoSuchElementException();
}
String cn = nextName;
nextName = null;
try {
S p = service.cast(Class.forName(cn, true, loader)
.newInstance());
providers.put(cn, p);
return p;
} catch (ClassNotFoundException x) {
fail(service,
Provider + cn + not found);
} catch (Throwable x) { // --- this could be an
instance of Error
fail(service,
Provider + cn + could not be instantiated: + x,
x);
}
throw new Error(); // This cannot happen
}
Class.forName
Re: Review Request: 7193683: Cleanup Warning in java.sql package
Looks good!
While you are there maybe correct indentations and tab - spaces, e.g.:
531 } catch(Throwable t) {
532 // Do nothing
533 }
But here maybe better:
531 } catch(Throwable t) {} // Do nothing
-Ulf
Am 29.08.2012 07:29, schrieb Dan Xu:
I made a simple fix to clean up build warnings in java.sql package. The change can be reviewed at
http://cr.openjdk.java.net/~dxu/7193683/webrev.01/. Thanks!
-Dan
Difference between Files#delete(Path) and File#delete()
Hi all, I'm not sure whether this is the right place for this question but if in case its not please direct me to the correct forum. I was trying to shift from old file API to NIO API (using JDK 7 update 6) but I found that on Windows (I have Windows 7) platform if I have read only file then File#delete() call will delete the file while Files#delete(Path) fails with exception (java.nio.file.AccessDeniedException), I'm not sure whether this is known issue but since Files#delete(Path) offers better error handling I would like to use Files#delete(Path). My question is - Is there any workaround for this problem in NIO API or should I stick to old File API because of this. Also I was curios why Files#delete(Path) throws exception but File#delete() executes successfully. PS: I asked this question[1] on stackoverflow but did not get any good response. 1. http://stackoverflow.com/questions/12139482/difference-between-filesdeletepath-and-filedelete/ -- Premraj
Re: Difference between Files#delete(Path) and File#delete()
On 29/08/2012 11:15, Premraj wrote: I was trying to shift from old file API to NIO API (using JDK 7 update 6) but I found that on Windows (I have Windows 7) platform if I have read only file then File#delete() call will delete the file while Files#delete(Path) fails with exception (java.nio.file.AccessDeniedException), I'm not sure whether this is known issue but since Files#delete(Path) offers better error handling I would like to use Files#delete(Path). My question is - Is there any workaround for this problem in NIO API or should I stick to old File API because of this. Also I was curios why Files#delete(Path) throws exception but File#delete() executes successfully. I would expect that if you try to delete the file in Explorer or in a command window that you also get an Access denied error. The issue is that java.io.File has many oddities, on Windows in particular. In this case it resets the file attributes before it deletes the file so this is why it does not fail as might be expected. It is behavior that dates back 10 years and so would be risky to change now. It has several other oddities like this, just one of the reason why it wasn't re-implemented to use the new API. -Alan.
Re: Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io
Am 24.08.2012 02:12, schrieb Andrew Hughes:
In FilePermission.java file, I make one change to its method
signature,
public Enumeration elements() == public EnumerationPermission
elements()
I am not sure whether it will cause an issue of backward
compatibility.
Please advise. Thanks!
Actually the whole method is synchronized. To make this more clear, I suggest:
798 public synchronized EnumerationPermission elements() {
799 // Convert Iterator into Enumeration
800 return Collections.enumeration(perms);
801 }
It's in a package-private class so I doubt it.
Even if it wasn't, a new major release is the perfect time to fix these issues.
If this class is package-private, why the constructor is public?
(please also check all other methods, especially if not inherited)
Please check indentations and tabs - spaces, while you are here.
E.g. following lines should be:
713 final class FilePermissionCollection extends PermissionCollection
714 implements Serializable {
732 * @param permission the Permission object to add.
733 *
734 * @throws IllegalArgumentException - if the permission is not a
743 if (! (permission instanceof FilePermission))
744 throw new IllegalArgumentException(
745 invalid permission: +permission);
746 if (isReadOnly())
747 throw new SecurityException(
748 attempt to add a Permission to a readonly
PermissionCollection);
767 if (! (permission instanceof FilePermission))
768 return false;
Lines 822..825 look ugly. Better:
819 /**
820 * @serialData permissions field (a Vector containing the
FilePermissions).
821 */
822 /*
823Writes the contents of the perms field out as a Vector for
824serialization compatibility with earlier releases.
825 */
or:
819 /**
820 * @serialData permissions field (a Vector containing the
FilePermissions).
821 */
822 // Writes the contents of the perms field out as a Vector for
823 // serialization compatibility with earlier releases.
I only took a short look on class FilePermission.java, but not the others of
this CR for now.
-Ulf
Re: Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io
Am 24.08.2012 21:07, schrieb Dan Xu:
On 08/23/2012 06:53 PM, David Holmes wrote:
I'm surprised that you need this:
426 @SuppressWarnings(fallthrough)
...
436 switch (actions) {
437 case SecurityConstants.FILE_READ_ACTION:
438 return READ;
Oops, you have reverted the change to use switch-on-Strings by webrev.03. Why?
If this generates a fallthrough warning then I think whatever tool generates that warning needs
fixing!
@SuppressWarnings(fallthrough) is put to suppress warnings generated by another switch/case
statements
Can't you move it from method scope to there?
while (i = matchlen !seencomma) {
switch(a[i-matchlen]) {
case ',':
seencomma = true;
/*FALLTHROUGH*/
case ' ': case '\r': case '\n':
case '\f': case '\t':
break;
default:
throw new IllegalArgumentException(
invalid permission: + actions);
}
i--;
}
-Ulf
Re: Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io
Am 29.08.2012 07:22, schrieb Dan Xu: 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! In class j.u.Collections you have added some @Override annotations. Why don't you add them in all appropriate cases and in all classes of your CR? As they are not very frequent in the JDK, I think we should leave it as it is or write a script, which patches the whole JDK in one chunk. -Ulf
Re: Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io
Hello, On 8/29/2012 1:48 AM, Rémi Forax wrote: On 08/29/2012 08:33 AM, Kurchi Subhra Hazra wrote: 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 Hi Dan, In PreHashedMap, the code should be Map.Entry?,? that = (Map.Entry?,?)ob; so the @SuppressWarnings is not needed anymore. And in java.util.Arrays, asList() should not be annotated with @SuppressWarnings because it is already annotated with SafeVarargs, so the compiler should not report the varargs warning. I have CC to compiler-dev to be sure that this is a compiler error. cheers, Rémi I've spoken to Stuart about this last point. The @SafeVarargs covers uses of Array.asList and covers the declaration of the asList method itself. However, it does not cover calls to the constructor inside the asList method, a constructor which itself is *not* @SafeVarargs. Solutions include * @SuppressWarnings(varargs) on the asList method * @SuppressWarnings(varargs) on new temporary variable for that purpose (with possible class file size consequences) * @SafeVarargs on the ArrayList constructor Regards, -Joe
Re: Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io
- Original Message - On 8/24/12 2:42 AM, Andrew Hughes wrote: However, once the whole build is warning free, would it not be preferable to remove these and just set JAVAC_WARNINGS_FATAL=true when doing development builds? The problem I see is someone new coming to OpenJDK and not being able to simply build it (with no changes) because a new warnings has appeared and is being treated as an error. This is less of a problem with javac as we control its development, and the JDK will use the javac built in the langtools step in most cases. But, generally, -Werror is something you should choose to enable, with the intention of fixing failures, not something that should be forced on everyone building the code. Our experience is that when -Werror is off by default, warnings tend to be introduced inadvertently. In the time we've been working on warnings cleanup, I've noticed the warnings count creeping up in areas of the build that don't have -Werror enabled. If it weren't for the implicit compilation issue, enabling -Werror incrementally as areas of the build are cleared would have been a good way to ensure that we make steady progress without any backsliding. But presumably these would be removed when everything is warning free? -Werror should not be the default for everyone building OpenJDK, who then end up having to fix or workaround issues which are nothing to do with them. It's easy enough for those who want to check we don't regress to turn on -Werror throughout, once everything is warning free. On a related topic, it would be nice if javadoc could also support -Werror as I constantly see warnings reappearing in the documentation. Yes, javadoc warnings are another *huge* problem. Yes, I see much more regression on this than code warnings (though I guess this is because most are hidden by javac's defaults). SUBDIRS_MAKEFLAGS += JAVAC_WARNINGS_FATAL=true and this is **not** overridable on the command line. That's wrong. If these are causing problems for you, please do submit patches. Yes, that's what we have in java/tools and is why JAVAC_WARNINGS_FATAL=false doesn't completely remove -Werror at present. I'll post a webrev of the change I have for this. Great, looking forward to this. s'marks -- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
Re: RFR [JDK8]: 7169894: JAXP Plugability Layer: using service loader
On Aug 29, 2012, at 10:56 AM, Paul Sandoz paul.san...@oracle.com wrote:
Hi Joe,
I would urge you to reconsider errors using SL, since SL is being explicitly
called out as part of the specification.
You can make any such SL-related errors more meaningful (yes, i want the
stack trace telling what bits of SL code were called!) and remove any
potential for CCEs, due to linkage errors [*], by passing the SCE instance to
the constructor of DatatypeConfigurationException or the
FactoryConfigurationError variants e.g.
244 } catch (ServiceConfigurationError e) {
245 throw new DatatypeConfigurationException(Provider +
className + could not be loaded or instantiated using
java.util.ServiceLoader, e);
Doh, please shoot me now, i was labouring under the misapprehension that SCE
was extending from Exception and not Error.
Apologies for the confusion.
Paul.
Re: Review Request: 7193683: Cleanup Warning in java.sql package
This looks fine. Dan, I will commit this for you Thursday Best Lance On Aug 29, 2012, at 1:29 AM, Dan Xu wrote: I made a simple fix to clean up build warnings in java.sql package. The change can be reviewed at http://cr.openjdk.java.net/~dxu/7193683/webrev.01/. Thanks! -Dan Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
Re: Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io
On 08/29/2012 08:27 AM, Joe Darcy wrote:
Hello,
On 8/29/2012 1:48 AM, Rémi Forax wrote:
On 08/29/2012 08:33 AM, Kurchi Subhra Hazra wrote:
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
Hi Dan,
In PreHashedMap, the code should be
Map.Entry?,? that = (Map.Entry?,?)ob;
so the @SuppressWarnings is not needed anymore.
And in java.util.Arrays, asList() should not be annotated with
@SuppressWarnings because it is already annotated with SafeVarargs,
so the compiler should not report the varargs warning.
I have CC to compiler-dev to be sure that this is a compiler error.
cheers,
Rémi
I've spoken to Stuart about this last point. The @SafeVarargs covers
uses of Array.asList and covers the declaration of the asList method
itself. However, it does not cover calls to the constructor inside
the asList method, a constructor which itself is *not* @SafeVarargs.
Solutions include
* @SuppressWarnings(varargs) on the asList method
* @SuppressWarnings(varargs) on new temporary variable for that
purpose (with possible class file size consequences)
* @SafeVarargs on the ArrayList constructor
Regards,
-Joe
The first change to the PreHashedMap is right, and I will update my fix.
For the second comment, as Joe mentioned, it is necessary. Otherwise,
the compiler will give the following warnings.
../../../src/share/classes/java/util/Arrays.java:2837: warning:
[varargs] Varargs method could cause heap pollution from
non-reifiable varargs parameter a
return new ArrayList(a);
^
../../../src/share/classes/java/util/Arrays.java:2837: warning:
[varargs] Varargs method could cause heap pollution from
non-reifiable varargs parameter a
return new ArrayList(a);
^
The first and second options works fine. But if I follow the third
option to add @SafeVarargs on the ArrayList constructor, I get the
following error.
../../../src/share/classes/java/util/Arrays.java:2849: error:
Invalid SafeVarargs annotation. Method ArrayList(E[]) is not a
varargs method.
ArrayList(E[] array) {
^
where E is a type-variable:
E extends Object declared in class ArrayList
So I tried further to change the constructor to a varargs method and add
the @SafeVarargs annotation, but the compiler still gives me warnings
like this,
../../../src/share/classes/java/util/Arrays.java:2837: warning:
[varargs] Varargs method could cause heap pollution from
non-reifiable varargs parameter a
return new ArrayList(a);
^
../../../src/share/classes/java/util/Arrays.java:2837: warning:
[varargs] Varargs method could cause heap pollution from
non-reifiable varargs parameter a
return new ArrayList(a);
^
../../../src/share/classes/java/util/Arrays.java:2852: warning:
[varargs] Varargs method could cause heap pollution from
non-reifiable varargs parameter array
a = array;
^
3 warnings
I think the current change is good for this one. Thanks for your comments.
-Dan
Re: RFR [JDK8]: 7169894: JAXP Plugability Layer: using service loader
I actually updated the webrev yesterday with your suggestion. Recall our discussions back in June, the suggestion was to delegate to the ServiceLoader, e.g. ServiceLoader.load(serviceClass). The rational was that the ServiceLoader uses context and then bootstrap class loader. The spec for public static S ServiceLoaderS load(ClassS service, ClassLoader loader) states: loader - The class loader to be used to load provider-configuration files and provider classes, or null if the system class loader (or, failing that, the bootstrap class loader) is to be used This is different. In JAXP, null is recognized as bootstrap classloader, but the ServiceLoader actually assumes it as system class loader. This will be a problem, whether or now a classloader is passed to the load method. --Joe On 8/29/2012 1:56 AM, Paul Sandoz wrote: For the SchemaFactoryFinder there is now a potentially subtle difference in what CL is used to load a class by SL and by SchemaFactoryFinder.createInstance. Previously the same CL was always used whatever the mechanism was used to identify the class name. Errors related to class loaders can be extremely hard to track down hence the conservative position. -- I will say no more on the above:-) Paul. [*] If a developer can declare a service provider then such linkage errors can potentially occur.
Re: Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io
On 8/29/12 4:36 AM, Ulf Zibis wrote:
436 switch (actions) {
437 case SecurityConstants.FILE_READ_ACTION:
438 return READ;
Oops, you have reverted the change to use switch-on-Strings by webrev.03. Why?
A fair question. I had instigated some internal conversation about reverting
this change, so here's the explanation. (Good catch, by the way.)
The original code was like this:
427 private static int getMask(String actions) {
...
435 // Check against use of constants (used heavily within the JDK)
436 if (actions == SecurityConstants.FILE_READ_ACTION) {
437 return READ;
438 } else if (actions == SecurityConstants.FILE_WRITE_ACTION) {
The various SecurityConstants being used here are Strings.
Note that this is doing String comparisons using == which is usually a bug. But
this code is relying on String interning to provide object identity for equal
string literals, in order to provide a fast path optimization for these common
cases. Changing this code to use switch-over-strings would be semantically
equivalent, but it would probably slow things down, since switch provides
equals() semantics instead of == semantics.
The permissions code is quite performance-sensitive, so I've asked Dan to
revert this change.
@SuppressWarnings(fallthrough) is put to suppress warnings generated by
another switch/case statements
Can't you move it from method scope to there?
while (i = matchlen !seencomma) {
switch(a[i-matchlen]) {
case ',':
seencomma = true;
/*FALLTHROUGH*/
case ' ': case '\r': case '\n':
case '\f': case '\t':
break;
default:
throw new IllegalArgumentException(
invalid permission: + actions);
}
i--;
}
Unfortunately there is no suitable place to put the annotation. Annotations can
only be placed on declarations, and the narrowest enclosing declaration around
this switch statement is, unfortunately, the entire method. One might consider
refactoring the switch statement into its own method, but that kind of
refactoring is too large a change for warnings cleanup.
s'marks
Re: RFR [JDK8]: 7169894: JAXP Plugability Layer: using service loader
Paul, Alan,
Confusion was what jaxp meant to give :) I was told that the
factory/object finders, security support classes were duplicated, and
needed to be kept in sync. But they are not even in their original
form, unfortunately.
Both of you mentioned that it's desirable to make SCE the cause for the
configuration exception. DatatypeConfigurationException is different
from all others in that, it's an Exception while others Error, it takes
Throwable as cause while others Exception. I did not want to change the
signatures of the other configuration error classes, that is, the
constructors would need to take Throwable instead of Exception (as they
should have already done).
As a comprise, I've wrapped the error message in a confguration error.
Would you think it's sufficient to add to the message what you did below
(e.g. could not be loaded or instantiated using java.util.ServiceLoader?
Or are you into making signature changes? (sth. I thought we didn't
want to for this patch)
--Joe
On 8/29/2012 8:56 AM, Paul Sandoz wrote:
On Aug 29, 2012, at 10:56 AM, Paul Sandozpaul.san...@oracle.com wrote:
Hi Joe,
I would urge you to reconsider errors using SL, since SL is being explicitly
called out as part of the specification.
You can make any such SL-related errors more meaningful (yes, i want the stack
trace telling what bits of SL code were called!) and remove any potential for
CCEs, due to linkage errors [*], by passing the SCE instance to the constructor
of DatatypeConfigurationException or the FactoryConfigurationError variants e.g.
244 } catch (ServiceConfigurationError e) {
245 throw new DatatypeConfigurationException(Provider + className +
could not be loaded or instantiated using java.util.ServiceLoader, e);
Doh, please shoot me now, i was labouring under the misapprehension that SCE
was extending from Exception and not Error.
Apologies for the confusion.
Paul.
Re: Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io
On 8/29/2012 4:20 PM, Stuart Marks wrote:
On 8/29/12 4:36 AM, Ulf Zibis wrote:
436 switch (actions) {
437 case SecurityConstants.FILE_READ_ACTION:
438 return READ;
Oops, you have reverted the change to use switch-on-Strings by
webrev.03. Why?
A fair question. I had instigated some internal conversation about
reverting this change, so here's the explanation. (Good catch, by the
way.)
The original code was like this:
427 private static int getMask(String actions) {
...
435 // Check against use of constants (used heavily within
the JDK)
436 if (actions == SecurityConstants.FILE_READ_ACTION) {
437 return READ;
438 } else if (actions == SecurityConstants.FILE_WRITE_ACTION) {
The various SecurityConstants being used here are Strings.
Note that this is doing String comparisons using == which is usually a
bug. But this code is relying on String interning to provide object
identity for equal string literals, in order to provide a fast path
optimization for these common cases. Changing this code to use
switch-over-strings would be semantically equivalent, but it would
probably slow things down, since switch provides equals() semantics
instead of == semantics.
This would be a fine point to highlight in the comments around this check!
-Joe
RFR: 7195099 update problem list with RMI test failures
Hi Alan, I'm opening another front in the war against test failures. Please review these additions to the problem list. Thanks. s'marks diff -r c4c69b4d9ace test/ProblemList.txt --- a/test/ProblemList.txt Wed Aug 29 11:03:02 2012 +0800 +++ b/test/ProblemList.txt Wed Aug 29 19:00:56 2012 -0700 @@ -261,6 +261,18 @@ # 7146541 java/rmi/transport/rapidExportUnexport/RapidExportUnexport.java linux-all +# 7187882 +java/rmi/activation/checkusage/CheckUsage.java generic-all + +# 7190106 +java/rmi/reliability/benchmark/runRmiBench.sh generic-all + +# 7191877 +java/rmi/transport/checkLeaseInfoLeak/CheckLeaseLeak.java generic-all + +# 7195095 +sun/rmi/transport/proxy/EagerHttpFallback.java linux-all + # jdk_security
Re: Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io
On 8/29/12 4:56 PM, Joe Darcy wrote:
On 8/29/2012 4:20 PM, Stuart Marks wrote:
The original code was like this:
427 private static int getMask(String actions) {
...
435 // Check against use of constants (used heavily within the JDK)
436 if (actions == SecurityConstants.FILE_READ_ACTION) {
437 return READ;
438 } else if (actions == SecurityConstants.FILE_WRITE_ACTION) {
The various SecurityConstants being used here are Strings.
Note that this is doing String comparisons using == which is usually a bug.
But this code is relying on String interning to provide object identity for
equal string literals, in order to provide a fast path optimization for these
common cases. Changing this code to use switch-over-strings would be
semantically equivalent, but it would probably slow things down, since switch
provides equals() semantics instead of == semantics.
This would be a fine point to highlight in the comments around this check!
The comment at line 435 did serve to tip me off that something unusual was
going on with this code, so it served its purpose. But I'll admit that it's not
as explicit as it could be. How about this:
// Use object identity comparisons with String constants for performance
// reasons (these values are used heavily within the JDK).
s'marks
Re: Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io
Hi Stuart, On 30/08/2012 8:53 AM, Stuart Marks wrote: On 8/29/12 8:48 AM, Andrew Hughes wrote: But presumably [-Werror] would be removed when everything is warning free? -Werror should not be the default for everyone building OpenJDK, who then end up having to fix or workaround issues which are nothing to do with them. It's easy enough for those who want to check we don't regress to turn on -Werror throughout, once everything is warning free. Once everything is warning free, -Werror *should* be enabled by default. That's how we'll keep everything warning free. If everything were warning free, I don't see how people could be blocked by warnings that are unrelated to their current changes. This does happen today with implicit compilation, but it shouldn't happen in the bright shining future where there are no warnings. :-) It will happen to the first person who updates gcc and encounters the next round of gcc enhancements that cause our old C++ code to now generate warnings. If there's some other phenomenon that could cause warnings to pop up spontaneously, unrelated to their current changes, I'd like to understand what that is. gcc :) David s'marks
