date:20121214


Am 14.12.2012 03:15, schrieb Mandy Chung:

JDepsTask:111
Did you mean --summary?


Yes will fix it.


Why don't you remain consistent to all other existing java tools?
E.g. javac uses: -cp path or -classpath path
Double hyphen '--' is never used until today.

So better:
  -P  -profileShow profile or the file containing a package
  -R  -recursive  Traverse all dependencies recursively

Anyway, if you prefer to stick at '--', then you should consistently use it for '--version', 
'--classpath', '--all'


-Ulf


  -version Version information
  -classpath pathSpecify where to find class files
  -summary Print dependency summary only
  -v:class Print class-level dependencies
  -v:package   Print package-level dependencies
  -p package nameRestrict analysis to classes in this package
   (may be given multiple times)
  -e regex   Restrict analysis to packages matching pattern
   (-p and -e are exclusive)
  -P  --profileShow profile or the file containing a package
  -R  --recursive  Traverse all dependencies recursively
  -all Process all classes specified in -classpath

Re: RFR: 8005051: default methods for Iterator

2012-12-14 Thread Remi Forax


Hi Akhil,

On 12/14/2012 02:24 AM, Akhil Arora wrote:

As part of the library lambdafication, this patch adds a forEach default
method to Iterator, and converts remove() into a default method so that
implementations of Iterator no longer have to override remove if they
desire the default behavior, which is to throw an
UnsupportedOperationException.

http://cr.openjdk.java.net/~akhil/8005051.0/webrev/

The above patch requires a small patch to an internal class which
happens to implement both Iterable and Iterator. Now both Iterable and
Iterator supply a default forEach method, so the compiler balks. One
minimally intrusive solution is for this class to override both defaults
and provide its own version of forEach.

http://cr.openjdk.java.net/~akhil/8005053.0/webrev/


The issue here is in the code of ASM wich is an external library 
imported in the JDK,

so not sure it's a good idea to patch it.

Moreover, goggling for implement Iterable, Iterator shows that this 
anti-pattern is used too frequently to not step back and see if a better 
solution is not possible.

https://encrypted.google.com/search?hl=enq=%22implements+Iterable%3C*%3E%2C+Iterator%3C*%3E%22

We can't remove Collection.forEach without having perf issue because the 
stream pipeline use it.
Iterator.forEach can be removed but it's a pity because it's really 
convenient,

a possble solution is to rename Iterator.forEach() to something else.



Please review
Thanks



cheers,
Rémi

Re: RFR: 8005051: default methods for Iterator

2012-12-14 Thread Chris Hegarty




On 14/12/2012 01:24, Akhil Arora wrote:

As part of the library lambdafication, this patch adds a forEach default
method to Iterator, and converts remove() into a default method so that
implementations of Iterator no longer have to override remove if they
desire the default behavior, which is to throw an
UnsupportedOperationException.


This is not specified. Different Java SE implementations can have 
different default implementations of remove(). Your statement is either 
mistaken, or the changes will not meet their original intent.


forEach, it is not clear to me if the method description is specifying 
what the default implementation is supposed to do, or if it is 
specifying the contract for the method.


This, of course, has the same underlying cause as what is being 
discussed in other mail threads. I would like to see a clear decision 
being made around how default methods are to be specified before we 
start moving them into jdk8.


-Chris.



http://cr.openjdk.java.net/~akhil/8005051.0/webrev/

The above patch requires a small patch to an internal class which
happens to implement both Iterable and Iterator. Now both Iterable and
Iterator supply a default forEach method, so the compiler balks. One
minimally intrusive solution is for this class to override both defaults
and provide its own version of forEach.

http://cr.openjdk.java.net/~akhil/8005053.0/webrev/

Please review
Thanks

Re: Review request: 8003562: Provide a command-line tool to find static dependencies


Some nits again:

_private_ static class Options has _public_ members, why?

IMHO, defining a distinct inner class for each Option is a little bit oversized. I also do not see 
the need for class Options, as it is only instantiated once. Instead you could define:

enum Options
Anyway, isn'd there still an options parser from other java langtools, which 
could be reused ???

In many places the source is exhausting to read, e.g
if (f.exists()) {
ClassFileReader reader = ClassFileReader.newInstance(f);
Archive archive = new Archive(f, reader);
result.add(archive);
}
could simply be:
if (f.exists())
result.add(new Archive(f, ClassFileReader.newInstance(f)));

... also spreading around the variable definitions at different places.

-Ulf


Am 14.12.2012 08:22, schrieb Mandy Chung:

Updated webrev:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/jdeps/webrev.04/

The binary name of an inner class has a '$' and so ClassFileReader.java:74 should do it.  Once the 
jdeps change is pulled down to the profiles forest, I may make the change there to use the API as 
javac uses.


thanks
Mandy

On 12/13/2012 6:15 PM, Mandy Chung wrote:

On 12/13/2012 4:06 PM, Jonathan Gibbons wrote:

Mandy,

Mostly good -- and nice to see Dependencies getting the full tool treatment.



Thanks for the review, Jon.


-- Jon


Archive.java:128
non-localized text: not found



Oh I missed that - will fix it.


ClassFileReader.java:74, ditto similar code in JarFileReader
Will not work for inner classes.



That's right.  What's the best way handling inner classes besides retrying?


ClassFileReader.java:various
Langtools is (for a while longer) still using -source 6. This is to allow NetBeans to build and 
use langtools. I guess the code here is OK as long as NB don't try and build all of langtools.




We talked about this and I hope that some part of langtools could be built with -source 7 as they 
are not needed in the bootstrap phase.  I will fix it since it's close to integration.   That'll 
help when you use NB to open langtools repo; otherwise, NB will indicate the files with syntax 
error which is kind of annoying.




JDepsTask:111
Did you mean --summary?


Yes will fix it.

If you're wanting to emulate the GNU-style --options, these options normally use =, as in 
--name=value, so you might want to update handleOption.




That's right - will fix it.


PlatformClassPath
The API you are looking for is now available in the profiles forest, in 
langtools/src/classes/com/sun/tools/javac/sym


Good - I'll check that out and send out a new webrev.

Thanks
Mandy



-- Jon

On 12/10/2012 02:30 AM, Erik Joelsson wrote:

Looks good.

/Erik

On 2012-12-07 22:55, Mandy Chung wrote:

This is the updated webrev. It includes Erik's makefile changes and
small change - be consistent with javap, jdeps can take classnames as
the input argument or wildcard * to analyze all class files that
replaces the -all option.

Webrev:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/jdeps/webrev.03/

Mandy

On 12/5/12 5:36 PM, Mandy Chung wrote:

This review request (add a new jdeps tool in the JDK) include changes in
langtools and the jdk build.  I would need reviewers from the langtools
and the build-infra team.

Webrev for review:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/jdeps/langtools-webrev.02/
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/jdeps/jdk-webrev.02/

The jdeps source is in the langtools repo.  The change in the jdk repo is
to add the new jdeps executable.  I made change in the old and new build
for the addition of this new jdeps tool.  I discussed with Jon whether I
should modify langtools/make/build.xml to add a new jdeps target. Since
the old build will be replaced by the new build soon, I simply add
com/sun/tools/jdeps as part of javap.includes.

Alan,

The -d option is kept as a hidden option and use as implementation. We
can remove it when it's determined not useful in the future. I also
rename -v:summary to -summary.

Thanks
Mandy



$ jdep -h
Usage: jdeps options files
where possible options include:
  -version Version information
  -classpath pathSpecify where to find class files
  -summary Print dependency summary only
  -v:class Print class-level dependencies
  -v:package   Print package-level dependencies
  -p package nameRestrict analysis to classes in this package
   (may be given multiple times)
  -e regex   Restrict analysis to packages matching pattern
   (-p and -e are exclusive)
  -P  --profileShow profile or the file containing a package
  -R  --recursive  Traverse all dependencies recursively
  -all Process all classes specified in -classpath

$ jdep Notepad.jar Ensemble.jar
Notepad.jar - D:\tools\devtools\jdk8\windows-i586\jre\lib\rt.jar

Re: Review request: JDK-8004928 TEST_BUG: Reduce dependence of CoreLib tests from the AWT subsystem (ver. 3)

2012-12-14 Thread Alexey Utkin


On 13.12.2012 20:59, Alan Bateman wrote:

One other one that I ran into today is this one:
test/java/lang/Throwable/LegacyChainedExceptionSerialization.java

It uses java.awt.print.PrinterIOException and I think that can be 
safely removed. Do you think this could be included in your changes?

Done. A PrinterIOException instance was removed from tested sequence.

Bug description:
https://jbs.oracle.com/bugs/browse/JDK-8004928

Here is the suggested fix:
http://cr.openjdk.java.net/~uta/openjdk-webrevs/JDK-8004928/webrev.03

Regards,
-uta

Re: RFR: 8005051: default methods for Iterator


On 14/12/2012 01:24, Akhil Arora wrote:
As part of the library lambdafication, this patch adds a forEach 
default method to Iterator, and converts remove() into a default 
method so that implementations of Iterator no longer have to override 
remove if they desire the default behavior, which is to throw an 
UnsupportedOperationException.


http://cr.openjdk.java.net/~akhil/8005051.0/webrev/

I looked at the changes to Iterator, a few minor comments:

I think it would help to change This default implementation to The 
default implementation, it makes it more obviously normative (and so 
testable) and might help for sub-types that override the method but 
don't inherit the javadoc.


I assume the generated javadoc ends as a long paragraph, did you 
consider putting in p tags to make it a bit easier on the reader, 
minimally put the description of the default implementation isn't own 
paragraph.


Should Collection have a lowercase c as it may be an iterator over a 
collection that is not a java.util.Collection?


In forEach then it may be smoother to change ... execution subsequent 
... to ... execution then subsequent 


As per the other thread, if new methods are coming with the public 
modifier then I think it should be added to the existing methods.


-Alan.

Re: Review request: JDK-8004928 TEST_BUG: Reduce dependence of CoreLib tests from the AWT subsystem (ver. 3)


On 14/12/2012 12:14, Alexey Utkin wrote:

On 13.12.2012 20:59, Alan Bateman wrote:

One other one that I ran into today is this one:
test/java/lang/Throwable/LegacyChainedExceptionSerialization.java

It uses java.awt.print.PrinterIOException and I think that can be 
safely removed. Do you think this could be included in your changes?

Done. A PrinterIOException instance was removed from tested sequence.

Bug description:
https://jbs.oracle.com/bugs/browse/JDK-8004928

Here is the suggested fix:
http://cr.openjdk.java.net/~uta/openjdk-webrevs/JDK-8004928/webrev.03

Regards,
-uta


Thanks for including this one, thumbs up from me.

-Alan

Re: Review request: 8003562: Provide a command-line tool to find static dependencies


On 14/12/2012 07:22, Mandy Chung wrote:
Once the jdeps change is pulled down to the profiles forest, I may 
make the change there to use the API as javac uses.


That would be fine, alternatively when the profiles changes get to jdk8 
and jdk8/tl.


-Alan.

Re: RFR: 8005051: default methods for Iterator


On Dec 14, 2012, at 7:28 AM, Alan Bateman wrote:

 On 14/12/2012 01:24, Akhil Arora wrote:
 As part of the library lambdafication, this patch adds a forEach default 
 method to Iterator, and converts remove() into a default method so that 
 implementations of Iterator no longer have to override remove if they desire 
 the default behavior, which is to throw an UnsupportedOperationException.
 
 http://cr.openjdk.java.net/~akhil/8005051.0/webrev/
 I looked at the changes to Iterator, a few minor comments:
 
 I think it would help to change This default implementation to The default 
 implementation, it makes it more obviously normative (and so testable) and 
 might help for sub-types that override the method but don't inherit the 
 javadoc.
 
 I assume the generated javadoc ends as a long paragraph, did you consider 
 putting in p tags to make it a bit easier on the reader, minimally put the 
 description of the default implementation isn't own paragraph.

This is why i am a proponent of adding a javadoc tag for default methods so 
that everyone is consistent in where/how we document the default method.  I 
worry if we do not developers will place this info in various places making it 
easy to overlook.
 
 Should Collection have a lowercase c as it may be an iterator over a 
 collection that is not a java.util.Collection?
 
 In forEach then it may be smoother to change ... execution subsequent ... 
 to ... execution then subsequent 
 
 As per the other thread, if new methods are coming with the public modifier 
 then I think it should be added to the existing methods.
 
 -Alan.
 
 


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: RFR: 8005051: default methods for Iterator

2012-12-14 Thread Paul Sandoz

On Dec 14, 2012, at 11:54 AM, Remi Forax fo...@univ-mlv.fr wrote:
 
 We can't remove Collection.forEach without having perf issue because the 
 stream pipeline use it.
 Iterator.forEach can be removed but it's a pity because it's really 
 convenient,

And a case can be made performance wise too (there are optimal implementations 
in the stream code base). 

I have found that forEach, in general, has been very useful. Since it abstracts 
away the details of traversal it has enabled better re-use of code.

Paul.

 a possble solution is to rename Iterator.forEach() to something else.
 
 
 Please review
 Thanks
 
 
 cheers,
 Rémi

Re: RFR: 8005051: default methods for Iterator

2012-12-14 Thread Remi Forax


On 12/14/2012 02:00 PM, Paul Sandoz wrote:

On Dec 14, 2012, at 11:54 AM, Remi Forax fo...@univ-mlv.fr wrote:

We can't remove Collection.forEach without having perf issue because the stream 
pipeline use it.
Iterator.forEach can be removed but it's a pity because it's really convenient,

And a case can be made performance wise too (there are optimal implementations 
in the stream code base).

I have found that forEach, in general, has been very useful. Since it abstracts 
away the details of traversal it has enabled better re-use of code.


The VM profiles the iterator when calling hasNext and next, if you end 
up by using Iterator.forEach instead of one of its overriden methods, 
you share the profile between part of the pipeline that should not be 
shared. So using Iterator.forEach may be less performant or not 
depending if forEach is overriden by a specific implementation or not.




Paul.


Rémi




a possble solution is to rename Iterator.forEach() to something else.


Please review
Thanks


cheers,
Rémi

Re: signatures that are recorded for default methods

2012-12-14 Thread Peter Levart


On 12/14/2012 01:41 PM, Ricky Clarkson wrote:


Surely a default method that all subclasses are instructed to override 
just shouldn't exist, right?


Then the compiler will 'instruct' subtypes to implement it 
automatically (i.e., by failing to compile).


I just wanted to illustrate how a sub-optimal specification of default 
method behaviour for optional methods would sound like if one didn't 
want to specify that it always throws UnsupportedOperationException ;-)


Peter
On Dec 14, 2012 6:42 AM, Peter Levart peter.lev...@gmail.com 
mailto:peter.lev...@gmail.com wrote:


On 12/14/2012 10:06 AM, Peter Levart wrote:
 On 12/14/2012 07:21 AM, David Holmes wrote:
 Paul,

 On 14/12/2012 9:46 AM, Paul Benedict wrote:
 Lance,

 Good questions. Someone with authority will surely answer, but
here's
 my armchair opinion...

 If the Javadoc is to specify how the default method executes, than
 that would naturally infer all default implementations must have a
 stated contract. You can't document the default execution
behavior in
 the public API and then let a provider switch the behavior.
The two go
 hand-in-hand, imo.

 I couldn't really make sense of that. :) The method has a contract.
 The default implementation has to honor that contract. The
question
 is whether how the default implementation honors the method
 contract is required to be part of a second contract.

 I hope that made sense :)
 I think that the answer is obvious. A default method provider
has only
 as much freedom in choosing the implementation of the default method
 that particular implementation differences among various
providers are
 not observable by the sane usage of the API. The only soft aspect
 might be performance. Any other behavioural difference should not be
 allowed. Otherwise there will be in-compatibilities among platform
 providers.

 For example, the default Iterator.remove() is implemented in
Oracle's
 JDK as always throwing UnsupportedOperationException. The TCK should
 test for that and the Javadoc should specify that.
Ok, I admit that in this particular case and other similar cases
(where
the default method does nothing useful), the specification could
alternatively specify: The default method behaviour is
unspecified and
useless. Don't call that method or make sure it is always
overridden if
you call it - the TCK in that case doesn't test the behaviour of such
method.

Peter

 As Joe said, default interface methods are no different than any
other
 overridable methods. They have a contract and behaviour. The
behaviour
 can be changed (overriden) within constraints defined by
contract, but
 the behaviour itself should also be specified and followed by
 different providers.

 Just my 2 cents.

 Regards, Peter


 David
 -


 Paul

 On Thu, Dec 13, 2012 at 5:30 PM, Lance Andersen - Oracle
 lance.ander...@oracle.com mailto:lance.ander...@oracle.com
 wrote:

 On Dec 13, 2012, at 6:16 PM, Leonid Arbuzov wrote:

 Good point, Joe.
 Those extra assertions for default methods can be checked
 by regular API tests separately from signature tests.

 I am not clear on this.  See the message attached from David
which
 ask a similar question (from the attached email):
 ---
 2. It is not obvious to me that the JDK's choice for a default
 implementation has to be _the_ only possible implementation
choice.
 In many/most cases there will be a very obvious choice, but that
 doesn't mean that all suppliers of OpenJDK classes have to be
 locked in to that choice.
 ---


 If everyone needs to implement the same default
implementation then
 great the JCK/TCK can test it and IMHO we should have a
javadoc tag
 for this.

 If everyone is free to choose what the default behavior is,
then we
 cannot test it.

 So has there been a decision WRT the requirements for default
methods?


 Best
 Lance
 Thanks,
 -leonid

 On 12/13/2012 1:05 PM, Joe Darcy wrote:
 Hello,

 As with concrete methods on abstract classes, I would
expect the
 specifications of the default methods to often contain text
akin
 to This default implementation does x, y, and z since if the
 method is to be called by subtypes, the self-use patterns
in the
 default method need to be known.

 Cheers,

 -Joe

 On 12/13/2012 11:24 AM, Leonid Arbouzov wrote:
 Hello Lance,

 My understanding would be that the signature test
 should check that interface method is marked as default method
 but do not track the code in its default body

Re: RFR: 8005051: default methods for Iterator

2012-12-14 Thread Vitaly Davidovich

One interesting thing for me here is .net's IEnumerable (akin to Iterable)
never received a ForEach extension method - this method only exists on
their ListT class. From what I gather this was to avoid passing a block
(Action in .net) that modifies the underlying collection (e.g removes an
item). I never really thought that paranoia was worth it since most uses
will not want to mutate. I'm glad Iterable will have this, but curious if
anyone thought about this case explicitly.

Thanks

Sent from my phone
On Dec 14, 2012 8:38 AM, Peter Levart peter.lev...@gmail.com wrote:

Hi,

Iterator.forEach(Block) does not specify anything about the order of
internal iteration in correspondence to the order of classical external
iteration (hasNext()/next()). I think that if the order must be the same
then Javadoc should mention it. If the orders are allowed be different,
then Javadoc should mention it too.

As for the clash with Iterable.forEach(Block): The name forEach suggests
just that - each element will be passed to Block. Nothing is said about the
order of elements or even Threads. I dont't think it should be, since
Iterables can be various kinds.

But Iterator is a one-shot single-threaded API and it's hard to imagine
the implementation where internal iteration would want to be different than
external. So the Iterator method be better called differently. What about
Iterator.iterate(Block) ?

Regards, Peter

On 12/14/2012 02:24 AM, Akhil Arora wrote:

As part of the library lambdafication, this patch adds a forEach default
method to Iterator, and converts remove() into a default method so that
implementations of Iterator no longer have to override remove if they
desire the default behavior, which is to throw an
UnsupportedOperationException.

http://cr.openjdk.java.net/~**akhil/8005051.0/webrev/http://cr.openjdk.java.net/~akhil/8005051.0/webrev/

The above patch requires a small patch to an internal class which happens
to implement both Iterable and Iterator. Now both Iterable and Iterator
supply a default forEach method, so the compiler balks. One minimally
intrusive solution is for this class to override both defaults and provide
its own version of forEach.

http://cr.openjdk.java.net/~**akhil/8005053.0/webrev/http://cr.openjdk.java.net/~akhil/8005053.0/webrev/

Please review
Thanks

Re: RFR: 8005051: default methods for Iterator

2012-12-14 Thread Zhong Yu

On Fri, Dec 14, 2012 at 4:54 AM, Remi Forax fo...@univ-mlv.fr wrote:
 Hi Akhil,

 On 12/14/2012 02:24 AM, Akhil Arora wrote:

 As part of the library lambdafication, this patch adds a forEach default
 method to Iterator, and converts remove() into a default method so that
 implementations of Iterator no longer have to override remove if they
 desire the default behavior, which is to throw an
 UnsupportedOperationException.

 http://cr.openjdk.java.net/~akhil/8005051.0/webrev/

 The above patch requires a small patch to an internal class which
 happens to implement both Iterable and Iterator. Now both Iterable and
 Iterator supply a default forEach method, so the compiler balks. One
 minimally intrusive solution is for this class to override both defaults
 and provide its own version of forEach.

 http://cr.openjdk.java.net/~akhil/8005053.0/webrev/


 The issue here is in the code of ASM wich is an external library imported in
 the JDK,
 so not sure it's a good idea to patch it.

 Moreover, goggling for implement Iterable, Iterator shows that this
 anti-pattern is used too frequently to not step back and see if a better
 solution is not possible.
 https://encrypted.google.com/search?hl=enq=%22implements+Iterable%3C*%3E%2C+Iterator%3C*%3E%22

 We can't remove Collection.forEach without having perf issue because the
 stream pipeline use it.
 Iterator.forEach can be removed but it's a pity because it's really
 convenient,
 a possble solution is to rename Iterator.forEach() to something else.

I agree with renaming the method; it shouldn't be confused with the
typical forEach() on a collection which visits *all* elements; here it
only visits the remaining elements.


 Please review
 Thanks


 cheers,
 Rémi

Re: Proposal: Fully Concurrent ClassLoading

2012-12-14 Thread Zhong Yu

On Wed, Dec 12, 2012 at 5:48 PM, David Holmes david.hol...@oracle.com wrote:
 On 13/12/2012 1:51 AM, Zhong Yu wrote:

 If a class loader is declared fully concurrent, yet
 getClassLoadingLock() is invoked, what's the harm of returning a
 dedicated lock anyway, exactly like what's done before?


 The whole point is to get rid of the lockMap and these lock objects.

 I could return a sentinel Object in all cases rather than null but that just
 seems to encourage continued use of something that shouldn't be used with a
 fully concurrent loader.

 Returning null versus throwing an exception is mostly a matter of style. I'd
 want to hear from anyone who invokes getClassLoadingLock() on any of the
 system classloaders to see which is preferable.

Since this change will break some applications, the failure should be
loud, with immediate and detailed information on what's going on, so
the poor developers can fix the problem quickly.

I had the pleasure of implementing classloaders before, and I think it
might be underestimated how many classloaders are actually out there.

Zhong


 Thanks for the comments.

 David


 On Tue, Dec 11, 2012 at 7:40 PM, David Holmesdavid.hol...@oracle.com
 wrote:

 On 11/12/2012 9:58 PM, Peter Levart wrote:


 On 12/11/2012 12:27 PM, David Holmes wrote:


 Peter,

 You are convincing me that all superclasses must be fully concurrent
 too. Otherwise we are just trying to second-guess a whole bunch of
 what-ifs. :)



 If you think some more, yes. The superclass might not use
 getClassLoadingLock() but rely on the fact that findClass() is allways
 called under a guard of per-class-name lock, for example. It's a matter
 of how far to go to prevent such miss-behaving fully-concurrent
 subclasses. So far to also prevent fully-concurrent subclasses that
 would otherwise be perfectly correct?

 Maybe not. Creating custom ClassLoaders is not an average programmer's
 job. Those that do this things will of course study the implementations
 of superclasses they extend and do the right thing. And it's reasonable
 to expect that they more or less will only extend JDK's ClassLoaders -
 but on the other hand if they only extend JDK's class loaders, they are
 not prevented to be fully-concurrent either way. Hm...



 Again I think it is just too hard to try and second-guess how a
 parallel-loader might rely on the per-class locks (I actually don't see
 any
 reasonable use for them beyond flow-control), and then how a concurrent
 loader subclass might need to modify things.

 If we simply disallow this then we can relax that constraint in the
 future
 if valid use-cases turn up for that capability. Of course if someone has
 a
 valid use-case during this discussion phase then of course that will
 influence the decision.

 Thanks,
 David

 Peter


 Thanks,
 David

 On 11/12/2012 7:44 PM, Peter Levart wrote:


 On 12/11/2012 10:29 AM, David Holmes wrote:


 On 11/12/2012 7:20 PM, Peter Levart wrote:


 On 12/11/2012 03:55 AM, David Holmes wrote:


 Question on the source code: registerAsFullyConcurrent has
 confusing
 comment -
 do the super classes all need to be parallel capable? Or do the
 super
 classes all need
 to be FullyConcurrent? I assume the latter, so just fix the
 comments.



 Actually it is the former. There's no reason to require that all
 superclasses be fully-concurrent. Of course a given loaders degree
 of
 concurrency may be constrained by what it's supertype allows, but
 there's no reason to actually force all the supertypes to be
 fully-concurrent: it is enough that they are at least all parallel
 capable.



 Hi David,

 There is one caveat: if ClassLoader X declares that it is
 fully-concurrent and it's superclass Y is only parallel-capable,
 then X
 will act as fully-concurrent (returning null from
 getClassLoadingLock()). superclass Y might or might not be coded to
 use
 the getClassLoadingLock(). X therefore has to know how Y is coded.
 To be
 defensive, X could ask for Y's registration and declare itself as
 only
 parallel-capable if Y declares the same so that when Y is upgraded
 to be
 fully-concurrent, X would become fully-concurrent automatically. To
 support situations where the same version of X would work in two
 environments where in one Y is only parallel-capable and in the
 other Y
 is fully-concurrent, there could be a static API to retrieve the
 registrations of superclasses.



 I don't quite follow this. What code in the superclass are you
 anticipating that the subclass will use which relies on the lock? Or
 is this just an abstract what if scenario?



 This is more or less what if. There might be a subclass Y of say
 java.lang.ClassLoader that overrides loadClass or findClass, declares
 that it is parallel-capable and in the implementation of it's
 loadClass
 or findClass, uses getClassLoadingLock() to synchronize access to it's
 internal state. Now there comes class X extends Y that declares that
 it
 is fully-concurrent. Of course this will not work, X has

Re: 8004371: (props) Properties.loadFromXML needs small footprint XML parser as fallback when JAXP is not present


On 13/12/2012 21:46, Joe Wang wrote:

:

What I was thinking?!  I had the property dtd in mind that requires 
the elements all be in lower cases, so therefore I should ignore case 
and allow it -- any better logic than that? :-)


Fixed now. And also added a couple of test cases.  Elements with wrong 
case would still have been rejected, but the error messages would be 
different.

I've put an updated webrev here:

http://cr.openjdk.java.net/~alanb/8004371/webrev.03/

This has the equalsIgnoreCase - case change to address the issue that 
Paul spotted.


I've also liberated two tests for this area that we had elsewhere. They 
are moved into un-changed for now and will exercise the new code when 
run the smallest profile.


Joe - if you can get your additional tests into a form that can be 
included in the jdk repository then we can include them in this push, 
alternatively you can do it as a follow-up change-set that expands the 
test coverage.


-Alan.

Re: Review request: JDK-8004928 TEST_BUG: Reduce dependence of CoreLib tests from the AWT subsystem (ver. 3)


Looks good to me.
Mandy

On 12/14/2012 4:14 AM, Alexey Utkin wrote:

On 13.12.2012 20:59, Alan Bateman wrote:

One other one that I ran into today is this one:
test/java/lang/Throwable/LegacyChainedExceptionSerialization.java

It uses java.awt.print.PrinterIOException and I think that can be 
safely removed. Do you think this could be included in your changes?

Done. A PrinterIOException instance was removed from tested sequence.

Bug description:
https://jbs.oracle.com/bugs/browse/JDK-8004928

Here is the suggested fix:
http://cr.openjdk.java.net/~uta/openjdk-webrevs/JDK-8004928/webrev.03

Regards,
-uta

RFR 8005081: java/util/prefs/PrefsSpi.sh fails on macos-x

2012-12-14 Thread Chris Hegarty

Strangely, this test passed in my test runs, on all platforms, before 
the push with the changes that pass the TESTVMOPTS, 8003890. It has now 
been seen to fail on some mac machines. There appears to be an issue 
with the use of quotes around TESTVMOPTS. The below change resolves the 
failure on the problem machines, and also continues to pass on all other 
platforms.


-
diff -r 8d7323a9d8ed test/java/util/prefs/PrefsSpi.sh
--- a/test/java/util/prefs/PrefsSpi.sh  Thu Dec 13 21:18:27 2012 -0500
+++ b/test/java/util/prefs/PrefsSpi.sh  Fri Dec 14 16:36:17 2012 +
@@ -87,17 +87,17 @@ Sys $javac -d jarDir StubPreferencesFa

 case `uname` in Windows*|CYGWIN* ) CPS=';';; *) CPS=':';; esac

-Sys $java ${TESTVMOPTS} -cp $TESTCLASSES${CPS}extDir/PrefsSpi.jar \
+Sys $java ${TESTVMOPTS} -cp $TESTCLASSES${CPS}extDir/PrefsSpi.jar \
 -Djava.util.prefs.PreferencesFactory=StubPreferencesFactory \
 -Djava.util.prefs.userRoot=. \
 PrefsSpi StubPreferences
-Sys $java ${TESTVMOPTS} -cp $TESTCLASSES \
+Sys $java ${TESTVMOPTS} -cp $TESTCLASSES \
 -Djava.util.prefs.userRoot=. \
 PrefsSpi java.util.prefs.*
-Sys $java ${TESTVMOPTS} -cp $TESTCLASSES${CPS}extDir/PrefsSpi.jar \
+Sys $java ${TESTVMOPTS} -cp $TESTCLASSES${CPS}extDir/PrefsSpi.jar \
 -Djava.util.prefs.userRoot=. \
 PrefsSpi StubPreferences
-Sys $java ${TESTVMOPTS} -cp $TESTCLASSES -Djava.ext.dirs=extDir \
+Sys $java ${TESTVMOPTS} -cp $TESTCLASSES -Djava.ext.dirs=extDir \
 -Djava.util.prefs.userRoot=. \
 PrefsSpi StubPreferences

-Chris.

Re: code review request : 8003147 port fix for BCEL bug 39695




On 12/12/2012 8:37 PM, David Buck wrote:

Hi Joe!

Thank you for looking at this.

 You didn't apply the original Apache completely. Was the omission of 
the

 change in toString() method intentional?

Yes, the improvement in toString() was not related to the issue and 
seems to have been included in the apache version of this fix on a 
whim. I did not feel it was appropriate to include it in the port as 
it is clearly outside of the scope of BCEL bug 39695.


Ok.



 I see that you've sent your test to Patrick. Have you run regression
 test for this patch?

I have only tested the different versions of my own testcase. I do not 
know anything about the test cases that Patrick is maintaining. If you 
or Patrick can walk me through how to get and run the test cases SQE 
uses, I will be more than happy to do that. Or if Patrick or you 
prefer, I can send you my build if it would be easier for one of you 
to run the tests yourself.


You may ask Patrick for instructions on how to run regression test.  It 
needs to be run separately since they are not in the jdk repo yet.


Thanks,
Joe



Cheers,
-Buck

On 2012/12/13 4:14, Joe Wang wrote:

Hi David,

You didn't apply the original Apache completely. Was the omission of the
change in toString() method intentional?

I see that you've sent your test to Patrick. Have you run regression
test for this patch?

Thanks,
Joe

On 12/9/2012 10:25 PM, David Buck wrote:

Hi!

I would like to request a code review of my JDK8 fix for the following
issue:

[ 8003147 : port fix for BCEL bug 39695 to our copy bundled as part of
jaxp ]
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8003147

In addition to the fix that the BCEL project had for this issue, I
needed to port some of their support for LocalVariableTypeTable:s to
our copy of BCEL as well. Implementing support for LVTT is very
straightforward and does not add much to the complexity or risk of
this change (especially considering the limited scope of jaxp's use of
BCEL).

Here is the webrev for my fix:

[ Code Review for jaxp ]
http://cr.openjdk.java.net/~dbuck/8003147/webrev.00/

My understanding is that the test cases for our copy of the jaxp code
are handled in a separate repository / test suite. I have been in
contact with Patrick Zhang (Oracle QA) and Joe Wang and have provided
a junit test for this issue as requested. Please see bug report for a
simpler (non-junit) test-case. If for some reason anyone wants to see
the junit-based test, please let me know and I can provide a copy of
that as well.

Best Regards,
-Buck

RFR: (jaxp) 8003260 : some fields should be package protected


Hi,

This is one of the three [findbug] issues. I've checked with Drew. None 
of them are vulnerabilities. Nonetheless, the fields should have been 
private or package private.


webrev:
http://cr.openjdk.java.net/~joehw/7u12/8003260/webrev/

Test:
No new test. Existing regression tests passed.

Thanks,
Joe

Re: RFR: (jaxp) 8003260 : some fields should be package protected

Hi Joe,

Shouldn't this also be private:

 static final char [] xmlDecl = {'','?','x','m','l'};

otherwise it is fine
Best
Lance
On Dec 14, 2012, at 1:33 PM, Joe Wang wrote:

 Hi,
 
 This is one of the three [findbug] issues. I've checked with Drew. None of 
 them are vulnerabilities. Nonetheless, the fields should have been private or 
 package private.
 
 webrev:
 http://cr.openjdk.java.net/~joehw/7u12/8003260/webrev/
 
 Test:
 No new test. Existing regression tests passed.
 
 Thanks,
 Joe


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: RFR : CR8004015 : Add parent interfaces and default methods to basic functional interfaces

2012-12-14 Thread Mike Duigou


On Dec 13 2012, at 22:28 , David Holmes wrote:

 I have added @throws NPE for a number of the default methods. We won't be 
 including @throws NPE in all cases where null is disallowed because when the 
 @throws NPE is declared the API is required to throw NPE in that 
 circumstance. So for cases where the NPE is naturally thrown or that 
 aren't performance sensitive we will likely add @throws NPE declarations but 
 for performance sensitive methods we won't be adding explicit null checks to 
 match a @throws NPE specification. There's a tradeoff here in some cases. 
 Please feel free to quibble about specific cases as they occur. :-)
 
 That doesn't make sense to me. The throwing of the NPE is intended to be part 
 of the specification not an implementation choice. Further @param foo 
 non-null, is just as binding on implementations as @throws NPE if foo is 
 null. ???

An @param foo non-null by itself is not considered normative because it 
doesn't document what happens when null is passed. So it ends up being advisory 
only. An @throws NPE is considered normative and the implementation must 
throw in all circumstances described. 

(Please bear with the step-by-step nature of the following sample, it's 
incremental pace is not meant to be insulting--it's a write-up for a general 
FAQ). If I have a method:

/**
*  If the object is visible then write it's string form to the provided 
PrintStream.
*
*  @param bar destination for display
/
public void display(PrintStream bar) {
 if(visible) {
   bar.write(toString());
 }
}

This implementation isn't going to work well if bar is ever null. So I 
document that in the @param bar:

/**
*  If the object is visible then write it's string form to the provided 
PrintStream.
*
*  @param bar destination for display, must be non-null
/
public void display(PrintStream bar) {
 if(visible) {
   bar.write(toString());
 }
}

The @param bar documentation now says that it must be non-null but doesn't 
explain what happens if null is passed. Having documented that null shouldn't 
be passed is helpful but not as helpful as it could be. To specify what happens 
I must add @throws NPE:

/**
*  If the object is visible then write it's string form to the provided 
PrintStream.
*
*  @param bar destination for display, must be non-null
*  @throws NullPointerException if bar is null
/
public void display(PrintStream bar) {
 if(visible) {
   bar.write(toString());
 }
}

This implementation would superficially seem to conform to the contract 
described in the javadoc. However, when the if(visible) conditional is false 
then no NPE will be thrown. Contract broken. It is required to add an explicit 
null check on bar ie.

/**
*  If the object is visible then write it's string form to the provided 
PrintStream.
*
*  @param bar destination for display, must be non-null
*  @throws NullPointerException if bar is null
/
public void display(PrintStream bar) {
 Objects.requireNonNull(bar);
 if(visible) {
   bar.write(toString());
 }
}

Adding the Objects.requireNonNull ensures that the NPE is thrown in all 
appropriate cases. There is also another alternative:

/**
*  If the object is visible then write it's string form to the provided 
PrintStream.
*
*  @param bar destination for display, must be non-null
*  @throws NullPointerException if bar is null and the component is visible.
/
public void display(PrintStream bar) {
 if(visible) {
   bar.write(toString());
 }
}

The conditions in which NPE are thrown are amended so that throwing is only 
required if the component is visible. Documenting the conditions could quickly 
become complicated if display was a more involved method. At some point it 
becomes easier to just add an explicit check. It can also be useful to add 
explicit NPE checks as pre-conditions before a multi-stage operation where a 
late stage NPE might corrupt the object state. 

In a very few cases an explicit null check might add too much overhead to a 
performance sensitive method and writing an accurate @throws NPE isn't 
possible (perhaps because of delegation) then the @throws NPE should be 
removed and only the advisory @param bar ... must be non-null will have to 
suffice.

Mike

 I think defining the NPE via the @param and @throws is a lose-lose situation:
 
 !  * @param left {@inheritDoc}, must be non-null
 !  * @param right {@inheritDoc}, must be non-null
 !  * @return {@inheritDoc}, always non-null
 !  * @throws NullPointerException if {@code left} or {@code right} is null
 
 You only need one convention.
 
 David
 -
 
 
 Mike

Re: RFR: (jaxp) 8003260 : some fields should be package protected




On 12/14/2012 10:36 AM, Lance Andersen - Oracle wrote:

Hi Joe,

Shouldn't this also be private:

  static final char [] xmlDecl = {'','?','x','m','l'};


A subclass in the same package, XMLDocumentScannerImpl, referred to it. 
That's why I made it package private.


Joe


otherwise it is fine
Best
Lance
On Dec 14, 2012, at 1:33 PM, Joe Wang wrote:


Hi,

This is one of the three [findbug] issues. I've checked with Drew. None of them 
are vulnerabilities. Nonetheless, the fields should have been private or 
package private.

webrev:
http://cr.openjdk.java.net/~joehw/7u12/8003260/webrev/

Test:
No new test. Existing regression tests passed.

Thanks,
Joe



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: RFR: (jaxp) 8003260 : some fields should be package protected

Thanks Joe.  maybe a quick comment would help in the code could be useful

Best
Lance
On Dec 14, 2012, at 2:49 PM, Joe Wang wrote:

 
 
 On 12/14/2012 10:36 AM, Lance Andersen - Oracle wrote:
 
 Hi Joe,
 
 Shouldn't this also be private:
 
  static final char [] xmlDecl = {'','?','x','m','l'};
 
 A subclass in the same package, XMLDocumentScannerImpl, referred to it. 
 That's why I made it package private.
 
 Joe
 
 otherwise it is fine
 Best
 Lance
 On Dec 14, 2012, at 1:33 PM, Joe Wang wrote:
 
 Hi,
 
 This is one of the three [findbug] issues. I've checked with Drew. None of 
 them are vulnerabilities. Nonetheless, the fields should have been private 
 or package private.
 
 webrev:
 http://cr.openjdk.java.net/~joehw/7u12/8003260/webrev/
 
 Test:
 No new test. Existing regression tests passed.
 
 Thanks,
 Joe
 
 
 Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
 Oracle Java Engineering 
 1 Network Drive 
 Burlington, MA 01803
 lance.ander...@oracle.com
 


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: RFR: (jaxp) 8003260 : some fields should be package protected


Thanks.  I added a comment.  Here's the webrev again:
http://cr.openjdk.java.net/~joehw/7u12/8003260/webrev/

Best
Joe

On 12/14/2012 11:52 AM, Lance Andersen - Oracle wrote:

Thanks Joe.  maybe a quick comment would help in the code could be useful

Best
Lance
On Dec 14, 2012, at 2:49 PM, Joe Wang wrote:




On 12/14/2012 10:36 AM, Lance Andersen - Oracle wrote:

Hi Joe,

Shouldn't this also be private:

  static final char [] xmlDecl = {'','?','x','m','l'};


A subclass in the same package, XMLDocumentScannerImpl, referred to 
it. That's why I made it package private.


Joe

otherwise it is fine
Best
Lance
On Dec 14, 2012, at 1:33 PM, Joe Wang wrote:


Hi,

This is one of the three [findbug] issues. I've checked with Drew. None of them 
are vulnerabilities. Nonetheless, the fields should have been private or 
package private.

webrev:
http://cr.openjdk.java.net/~joehw/7u12/8003260/webrev/

Test:
No new test. Existing regression tests passed.

Thanks,
Joe



Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com



http://oracle.com/us/design/oracle-email-sig-198324.gif
http://oracle.com/us/design/oracle-email-sig-198324.gifLance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com mailto:lance.ander...@oracle.com

Re: RFR: (jaxp) 8003260 : some fields should be package protected

+1
On Dec 14, 2012, at 3:43 PM, Joe Wang wrote:

 Thanks.  I added a comment.  Here's the webrev again:
 http://cr.openjdk.java.net/~joehw/7u12/8003260/webrev/
 
 Best
 Joe
 
 On 12/14/2012 11:52 AM, Lance Andersen - Oracle wrote:
 
 Thanks Joe.  maybe a quick comment would help in the code could be useful
 
 Best
 Lance
 On Dec 14, 2012, at 2:49 PM, Joe Wang wrote:
 
 
 
 On 12/14/2012 10:36 AM, Lance Andersen - Oracle wrote:
 
 Hi Joe,
 
 Shouldn't this also be private:
 
  static final char [] xmlDecl = {'','?','x','m','l'};
 
 A subclass in the same package, XMLDocumentScannerImpl, referred to it. 
 That's why I made it package private.
 
 Joe
 otherwise it is fine
 Best
 Lance
 On Dec 14, 2012, at 1:33 PM, Joe Wang wrote:
 
 Hi,
 
 This is one of the three [findbug] issues. I've checked with Drew. None 
 of them are vulnerabilities. Nonetheless, the fields should have been 
 private or package private.
 
 webrev:
 http://cr.openjdk.java.net/~joehw/7u12/8003260/webrev/
 
 Test:
 No new test. Existing regression tests passed.
 
 Thanks,
 Joe
 
 
 Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
 Oracle Java Engineering 
 1 Network Drive 
 Burlington, MA 01803
 lance.ander...@oracle.com
 
 
 
 Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
 Oracle Java Engineering 
 1 Network Drive 
 Burlington, MA 01803
 lance.ander...@oracle.com
 


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: 8003562: Provide a command-line tool to find static dependencies


I have cleaned up per your comments, Ulf.  Here is the updated webrev:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/jdeps/webrev.05/

This also cleans up how the inner classes are handled since 
Location.getClassName returns a fully-qualified classname.


On 12/14/2012 3:54 AM, Ulf Zibis wrote:

Some nits again:

_private_ static class Options has _public_ members, why?



Fixed.

IMHO, defining a distinct inner class for each Option is a little bit 
oversized. I also do not see the need for class Options, as it is only 
instantiated once. Instead you could define:

enum Options
Anyway, isn'd there still an options parser from other java langtools, 
which could be reused ???




I tried to be consistent with the javap implementation.  It's reasonable 
to have a common option parsing library for the command-line tools to 
use - for example [1] that we hope to get to it.



In many places the source is exhausting to read, e.g
if (f.exists()) {
ClassFileReader reader = ClassFileReader.newInstance(f);
Archive archive = new Archive(f, reader);
result.add(archive);
}
could simply be:
if (f.exists())
result.add(new Archive(f, ClassFileReader.newInstance(f)));

... also spreading around the variable definitions at different places.


Fixed.

Thanks
Mandy




Am 14.12.2012 08:22, schrieb Mandy Chung:

Updated webrev:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/jdeps/webrev.04/

The binary name of an inner class has a '$' and so 
ClassFileReader.java:74 should do it.  Once the jdeps change is 
pulled down to the profiles forest, I may make the change there to 
use the API as javac uses.


thanks
Mandy

http://hg.openjdk.java.net/jigsaw/jigsaw/jdk/file/ce66890e6d86/src/share/classes/org/openjdk/internal/joptsimple/ 


On 12/13/2012 6:15 PM, Mandy Chung wrote:

On 12/13/2012 4:06 PM, Jonathan Gibbons wrote:

Mandy,

Mostly good -- and nice to see Dependencies getting the full tool 
treatment.




Thanks for the review, Jon.


-- Jon


Archive.java:128
non-localized text: not found



Oh I missed that - will fix it.


ClassFileReader.java:74, ditto similar code in JarFileReader
Will not work for inner classes.



That's right.  What's the best way handling inner classes besides 
retrying?



ClassFileReader.java:various
Langtools is (for a while longer) still using -source 6. This is to 
allow NetBeans to build and use langtools. I guess the code here is 
OK as long as NB don't try and build all of langtools.




We talked about this and I hope that some part of langtools could be 
built with -source 7 as they are not needed in the bootstrap phase.  
I will fix it since it's close to integration.   That'll help when 
you use NB to open langtools repo; otherwise, NB will indicate the 
files with syntax error which is kind of annoying.




JDepsTask:111
Did you mean --summary?


Yes will fix it.

If you're wanting to emulate the GNU-style --options, these options 
normally use =, as in --name=value, so you might want to update 
handleOption.




That's right - will fix it.


PlatformClassPath
The API you are looking for is now available in the profiles 
forest, in langtools/src/classes/com/sun/tools/javac/sym


Good - I'll check that out and send out a new webrev.

Thanks
Mandy



-- Jon

On 12/10/2012 02:30 AM, Erik Joelsson wrote:

Looks good.

/Erik

On 2012-12-07 22:55, Mandy Chung wrote:

This is the updated webrev. It includes Erik's makefile changes and
small change - be consistent with javap, jdeps can take 
classnames as

the input argument or wildcard * to analyze all class files that
replaces the -all option.

Webrev:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/jdeps/webrev.03/

Mandy

On 12/5/12 5:36 PM, Mandy Chung wrote:
This review request (add a new jdeps tool in the JDK) include 
changes in
langtools and the jdk build.  I would need reviewers from the 
langtools

and the build-infra team.

Webrev for review:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/jdeps/langtools-webrev.02/ 

http://cr.openjdk.java.net/~mchung/jdk8/webrevs/jdeps/jdk-webrev.02/ 



The jdeps source is in the langtools repo.  The change in the 
jdk repo is
to add the new jdeps executable.  I made change in the old and 
new build
for the addition of this new jdeps tool.  I discussed with Jon 
whether I
should modify langtools/make/build.xml to add a new jdeps 
target. Since

the old build will be replaced by the new build soon, I simply add
com/sun/tools/jdeps as part of javap.includes.

Alan,

The -d option is kept as a hidden option and use as 
implementation. We

can remove it when it's determined not useful in the future. I also
rename -v:summary to -summary.

Thanks
Mandy



$ jdep -h
Usage: jdeps options files
where possible options include:
  -version Version information
  -classpath pathSpecify where to find class files
  -summary Print dependency summary only

hg: jdk8/tl/jaxp: 8003260: [findbug] some fields should be package protected

2012-12-14 Thread huizhe . wang

Changeset: b1fdb101c82e
Author:joehw
Date:  2012-12-14 13:24 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jaxp/rev/b1fdb101c82e

8003260: [findbug] some fields should be package protected
Summary: change public or protected mutable static fields to private or package 
private.
Reviewed-by: lancea

! 
src/com/sun/org/apache/xerces/internal/impl/XMLDocumentFragmentScannerImpl.java
! src/com/sun/org/apache/xerces/internal/impl/XMLEntityScanner.java

Re: Review request: 8003562: Provide a command-line tool to find static dependencies


Ulf,

Thanks for the review.

On 12/14/2012 6:13 AM, Ulf Zibis wrote:

Am 14.12.2012 11:47, schrieb Ulf Zibis:

Am 14.12.2012 03:15, schrieb Mandy Chung:

JDepsTask:111
Did you mean --summary?


Yes will fix it.


Why don't you remain consistent to all other existing java tools?
E.g. javac uses: -cp path or -classpath path
Double hyphen '--' is never used until today.



'--' is the GNU-style option.   pack200 uses GNU-style options for some 
time.  There was some discussion in jigsaw-dev [1] and the new jigsaw 
CLIs are moving to that direction.



So better:
  -P  -profileShow profile or the file containing a package
  -R  -recursive  Traverse all dependencies recursively

Anyway, if you prefer to stick at '--', then you should consistently 
use it for '--version', '--classpath', '--all'


Renamed to --version.



Maybe I'm in error, but additionally it seems to me, that 
-classpath=Foo doesn't match by:


-classpath is the one inconsistent with others. We think that it would 
be better to live with -classpath as people are familiar with 
-classpath option.


Thanks
Mandy
[1] http://mail.openjdk.java.net/pipermail/jigsaw-dev/2009-March/46.html

RFR: 4247235: (spec str) StringBuffer.insert(int, char[]) specification is inconsistent

2012-12-14 Thread Jim Gish

Please review 
http://cr.openjdk.java.net/~jgish/Bug4247235-Add-Blanket-Null-Stmt/ 
http://cr.openjdk.java.net/%7Ejgish/Bug4247235-Add-Blanket-Null-Stmt/


This minor spec change (which will require CCC approval), adds blanket 
null-handling statements to both StringBuffer and StringBuilder, 
equivalent to the one already in String.


Thanks,
   Jim

--
Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304
Oracle Java Platform Group | Core Libraries Team
35 Network Drive
Burlington, MA 01803
jim.g...@oracle.com

Re: Review request: 8003562: Provide a command-line tool to find static dependencies


Thanks for your explanations Mandy.

Am 14.12.2012 21:54, schrieb Mandy Chung:


On 12/14/2012 3:54 AM, Ulf Zibis wrote:

In many places the source is exhausting to read, e.g
if (f.exists()) {
ClassFileReader reader = ClassFileReader.newInstance(f);
Archive archive = new Archive(f, reader);
result.add(archive);
}
could simply be:
if (f.exists())
result.add(new Archive(f, ClassFileReader.newInstance(f)));

... also spreading around the variable definitions at different places.


Fixed.


Other examples:
===
 484 while (arg != null) {
  ...
 490 arg = iter.hasNext() ? iter.next() : null;
 491 }
Could be:
 484 for (; iter.hasNext(); arg = iter.next()) {
  ...
 491 }
===
 224 Dependency.Finder finder = Dependencies.getClassDependencyFinder();
  ...
 238 findDependencies(finder, filter);
  ...
 273 private void findDependencies(Dependency.Finder finder,
 274   Dependency.Filter filter)
Could be:
 238 findDependencies(filter);
  ...
 273 private void findDependencies(Dependency.Filter filter) {
 274 Dependency.Finder finder = Dependencies.getClassDependencyFinder();
at least remove line 224 and use:
 238 findDependencies(Dependencies.getClassDependencyFinder(), filter);
===


-Ulf

Re: Review request: 8003562: Provide a command-line tool to find static dependencies


Am 14.12.2012 22:41, schrieb Mandy Chung:



Maybe I'm in error, but additionally it seems to me, that -classpath=Foo 
doesn't match by:


-classpath is the one inconsistent with others. We think that it would be better to live with 
-classpath as people are familiar with -classpath option.


Ah, I see. But then you should still use:
 108 new Option(true, -cp, -classpath) {

Additionally, why not providing both, the old + new syntax:
 108 new Option(true, -cp, -classpath, --classpath) {

People are also still familiar with -version !

-Ulf

Re: Review request: 8003562: Provide a command-line tool to find static dependencies

2012-12-14 Thread Jonathan Gibbons


On 12/14/2012 04:44 PM, Ulf Zibis wrote:

Am 14.12.2012 22:41, schrieb Mandy Chung:


Maybe I'm in error, but additionally it seems to me, that 
-classpath=Foo doesn't match by:


-classpath is the one inconsistent with others. We think that it 
would be better to live with -classpath as people are familiar with 
-classpath option.


Ah, I see. But then you should still use:
 108 new Option(true, -cp, -classpath) {

Additionally, why not providing both, the old + new syntax:
 108 new Option(true, -cp, -classpath, --classpath) {

People are also still familiar with -version !

-Ulf



I guess I am against inconsistent use of the GNU option pattern. It is 
bad enough that we have to have different tools with different 
standards, but if we're going to a more GNU like pattern, we should have 
a more consistent policy about how we mix these styles. Inconsistently 
adopting GNU style is in my opinion worse than not adopting it.


-- Jon

Re: Review request: 8003562: Provide a command-line tool to find static dependencies