Re: [PATCH] Review Request for 8009258: TEST_BUG: java/io/pathNames/GeneralWin32.java fails intermittently

[Eric Wang]

Hi,

Please review the code change, I have updated the test to make sure test 
only access files and directories created by itself.

http://cr.openjdk.java.net/~ewang/8009258/webrev.01/

Here is the execution result:
http://cr.openjdk.java.net/~ewang/8009258/GeneralWin32.jtr

Thanks,
Eric
On 2013/3/5 1:39, Alan Bateman wrote:

On 04/03/2013 17:32, Eric Wang wrote:

Hi,

Please help to review fix below for bug 8009258 
, 
TEST_BUG:java/io/pathNames/GeneralWin32.java fails intermittently.

http://cr.openjdk.java.net/~ewang/8009258/webrev.00/

The File.canRead() method should not be used to check read permission 
of a directory.


Thanks,
Eric
I wonder if it would be better to change this test so that it doesn't 
even attempt to poke around in these directories. I suggest this 
because there may be other activity going on at the same time. See 
also 8004096 where the test is running in agentvm mode and is straying 
into the directory used by another agent VM.


-Alan

hg: jdk8/tl/jdk: 8009925: Back out AEAD CipherSuites temporarily

[bradford . wetmore]

Changeset: 6379415d8fca
Author:wetmore
Date:  2013-03-12 15:31 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/6379415d8fca

8009925: Back out AEAD CipherSuites temporarily
Reviewed-by: valeriep

! src/share/classes/com/sun/crypto/provider/TlsKeyMaterialGenerator.java
! src/share/classes/sun/security/internal/spec/TlsKeyMaterialParameterSpec.java
! src/share/classes/sun/security/internal/spec/TlsKeyMaterialSpec.java
! src/share/classes/sun/security/pkcs11/P11TlsKeyMaterialGenerator.java
- src/share/classes/sun/security/ssl/Authenticator.java
! src/share/classes/sun/security/ssl/CipherBox.java
! src/share/classes/sun/security/ssl/CipherSuite.java
! src/share/classes/sun/security/ssl/EngineInputRecord.java
! src/share/classes/sun/security/ssl/EngineOutputRecord.java
! src/share/classes/sun/security/ssl/EngineWriter.java
! src/share/classes/sun/security/ssl/Handshaker.java
! src/share/classes/sun/security/ssl/InputRecord.java
! src/share/classes/sun/security/ssl/JsseJce.java
! src/share/classes/sun/security/ssl/MAC.java
! src/share/classes/sun/security/ssl/OutputRecord.java
! src/share/classes/sun/security/ssl/Record.java
! src/share/classes/sun/security/ssl/SSLEngineImpl.java
! src/share/classes/sun/security/ssl/SSLSocketImpl.java
! test/sun/security/ec/TestEC.java
! test/sun/security/pkcs11/fips/CipherTest.java
! test/sun/security/pkcs11/sslecc/CipherTest.java
! 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLEngineImpl/SSLEngineBadBufferArrayAccess.java
- test/sun/security/ssl/javax/net/ssl/TLSv12/ShortRSAKeyGCM.java
! test/sun/security/ssl/sanity/ciphersuites/CipherSuitesInOrder.java
! test/sun/security/ssl/sanity/interop/CipherTest.java
! test/sun/security/ssl/templates/SSLSocketSSLEngineTemplate.java

Re: Review Request: JDK-8001334 - Remove use of JVM_* functions from java.io code

[Dan Xu]

I understand now. Here is the updated webrev to directly map IO_Append 
to handleWrite in *nix platforms, 
http://cr.openjdk.java.net/~dxu/8001334/webrev.03/.


I checked FileOutputStream.java source code, and we do guarantee the 
consistency of append flag between open and write operations. Thanks!


-Dan

On 03/12/2013 02:22 PM, Alan Bateman wrote:

On 12/03/2013 18:01, Dan Xu wrote:


Hi Alan,

Do you mean directly map IO_Append to handleWrite in io_util_md.h for 
the *nix case? And then where do we check the O_APPEND flag in our 
code? Or do we require users to open the file with O_APPEND flag? 
Thanks!
Yes, either IO_Append is defined to be handleWrite or else add 
handleAppend that simply calls handleWrite. There's no need to check 
O_APPEND after the file is opened for append, not on *nix anyway.


-Alan

Re: Review Request: JDK-8001334 - Remove use of JVM_* functions from java.io code

[Alan Bateman]

On 12/03/2013 18:01, Dan Xu wrote:


Hi Alan,

Do you mean directly map IO_Append to handleWrite in io_util_md.h for 
the *nix case? And then where do we check the O_APPEND flag in our 
code? Or do we require users to open the file with O_APPEND flag? Thanks!
Yes, either IO_Append is defined to be handleWrite or else add 
handleAppend that simply calls handleWrite. There's no need to check 
O_APPEND after the file is opened for append, not on *nix anyway.


-Alan

Re: Review Request: JDK-8001334 - Remove use of JVM_* functions from java.io code

[Dan Xu]

On 03/12/2013 08:19 AM, Alan Bateman wrote:

On 11/03/2013 23:43, Dan Xu wrote:
Thanks for all your comments. I have updated the fix accordingly. 
Please see the webrev at 
http://cr.openjdk.java.net/~dxu/8001334/webrev.02/.


For the language concern in getLastErrorString(char *buf, size_t len) 
function, I will log another bug and address it later. Thanks!


-Dan
You've addressed all my comments but I think I may have confused you 
on one point when I mentioned O_APPEND. You've changed handleAppend to 
use fcntl(F_GETFL) and check if the flag is set but this will happen 
on every write in append mode and we don't want that. I think you can 
simply change IO_Append to be handleWrite or else have handleAppend 
call handleWrite. The jboolean flag isn't needed for the *nix case.


-Alan.

Hi Alan,

Do you mean directly map IO_Append to handleWrite in io_util_md.h for 
the *nix case? And then where do we check the O_APPEND flag in our code? 
Or do we require users to open the file with O_APPEND flag? Thanks!


-Dan

hg: jdk8/tl/langtools: 7196531: Duplicate error messages on repeating annotations

[joel . franck]

Changeset: f427043f8c65
Author:jfranck
Date:  2013-03-12 17:39 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/langtools/rev/f427043f8c65

7196531: Duplicate error messages on repeating annotations
Reviewed-by: jjg

! src/share/classes/com/sun/tools/javac/comp/Annotate.java
+ test/tools/javac/annotations/repeatingAnnotations/DuplicateErrors.java
+ test/tools/javac/annotations/repeatingAnnotations/DuplicateErrors.out
! test/tools/javac/annotations/repeatingAnnotations/NoRepeatableAnno.out
! 
test/tools/javac/annotations/typeAnnotations/failures/common/arrays/DuplicateTypeAnnotation.out
! 
test/tools/javac/annotations/typeAnnotations/failures/common/innertypeparams/DuplicateTypeAnnotation.out
! 
test/tools/javac/annotations/typeAnnotations/failures/common/newarray/DuplicateTypeAnnotation.out
! 
test/tools/javac/annotations/typeAnnotations/failures/common/parambounds/DuplicateTypeAnnotation.out
! 
test/tools/javac/annotations/typeAnnotations/failures/common/receiver/DuplicateTypeAnnotation.out
! 
test/tools/javac/annotations/typeAnnotations/failures/common/rest/DuplicateTypeAnnotation.out
! 
test/tools/javac/annotations/typeAnnotations/failures/common/typeArgs/DuplicateTypeAnnotation.out
! 
test/tools/javac/annotations/typeAnnotations/failures/common/typeparams/DuplicateTypeAnnotation.out
! 
test/tools/javac/annotations/typeAnnotations/failures/common/wildcards/DuplicateTypeAnnotation.out
! 
test/tools/javac/annotations/typeAnnotations/newlocations/RepeatingTypeAnnotations.out

hg: jdk8/tl/langtools: 2 new changesets

[maurizio . cimadamore]

Changeset: 6db9a3b1a93f
Author:mcimadamore
Date:  2013-03-12 16:02 +
URL:   http://hg.openjdk.java.net/jdk8/tl/langtools/rev/6db9a3b1a93f

8008540: Constructor reference to non-reifiable array should be rejected
8008539: Spurious error when constructor reference mention an interface type
8008538: Constructor reference accepts wildcard parameterized types
Summary: Overhaul of Check.checkConstructorRefType
Reviewed-by: jjg

! src/share/classes/com/sun/tools/javac/comp/Check.java
! test/tools/javac/lambda/MethodReference38.out
+ test/tools/javac/lambda/MethodReference64.java
+ test/tools/javac/lambda/MethodReference64.out

Changeset: 5ddecb91d843
Author:mcimadamore
Date:  2013-03-12 16:02 +
URL:   http://hg.openjdk.java.net/jdk8/tl/langtools/rev/5ddecb91d843

8009545: Graph inference: dependencies between inference variables should be 
set during incorporation
Summary: Move all transitivity checks into the incorporation round
Reviewed-by: jjg

! src/share/classes/com/sun/tools/javac/code/Type.java
! src/share/classes/com/sun/tools/javac/comp/Infer.java
! test/tools/javac/lambda/TargetType28.out

Re: Code review request: JDK-8008670 (partial java.util.stream implementation)

[Paul Sandoz]

Hi Mike,


On Mar 12, 2013, at 2:29 AM, Mike Duigou  wrote:
> AbstractTask:: 
> 
> - "that describes a portion of the input" -> describes the he portion of the 
> input associated with the subtree rooted at this task.
> 
> - setRawResult() : needs better docs. Why does getRawResult return local 
> (which may be null)?
> 

I need to look into this more closely. I recall some subtle things related to 
CountedCompleter.complete/onCompletion methods and short-circuiting operations 
in terms of the order of invocation that caused us to side step 
get/setRawResult with get/setLocalResult.


> - Are trees of abstract task always homogeneous?

Yes.


> Perhaps an assert parent.getClass == class ? since the T can't be verified to 
> be the same as the class.
> 
> - onCompletion does not clear numChildren
> 

Currently it is designed to clear stuff to help GC, once the task is completed 
it should not be reused. So we should state something about reuse.


> 
> 
> ForEachOps::
> 
> - BooleanProvider -> BooleanSupplier
> 
> - in whatever Thread -> on whatever thread
> 
> - "There is no guarantee that additional elements.." should be before 
> @apiNote.
> 
> - Wouldn't it be nice if there as a way to template the primitive impls. :-)
> 

I am not sure it is worth it for 3 primitive types. There is also a cost of 
developing with templates, including that of the template system itself. 
Ideally i want a template system whereby one can implement the int version and 
annotate with descriptions that say how code can be substituted to produce 
long/double versions and we use javac with an annotation processor to generate 
the source as part of the compilation process.


> 
> IntermediateOp::
> 
> - "An intermediate operation has an input type and an output type". These 
> aren't reified or introspectable in contrast to the shape.
> 

Tricky. How would we do that for say:

  Stream> s = ...
  s.flatMap(s::stream)..   // List -> String

?


> - Perhaps defer more about statefulness to StatefulOp
> 

StatefulOp is a helper interface. It's not required to to implement a stateful 
operation, so perhaps we should clarify that point.


> 
> StatefulOp::
> 
> - Should include a sentence about how a stateful op knows when it has 
> received it's last element.
> 
> - Perhaps re-iterate wrapSink() to provide documentation about completion and 
> when elements are written to output. ie. Sink.end()
> 

Not all stateful ops accumulate state and wait until sink.end() to push that 
accumulated state. SortedOp does that but DistinctOp and SliceOp do not, as 
they accumulate "side-state" to determine when elements are pushed downstream 
or not.


> 
> Sink::
> 
> - ::end() "If the {@code Sink} buffers any results from previous values, they 
> should dump their contents downstream and clear any stored state." -> If the 
> {@code Sink} is stateful it should send all of it's stored state downstream.
> 
> - :: I wonder about the suggestion to clear stored state. Why is this better 
> than just letting it be GCed? If there is value to explicit clearing we 
> should explain why (at least partially).
> 

The idea being is Sink's can be resued. We don't currently do so, but we can 
consider an optimization later on whereby we cache a sink per fork/join thread.


> - ::cancellationRequested() -> :: accepting()? Cancellation sounds like an 
> exceptional state. 
> 
> - ::cancellationRequested() - document default. @implNote
> 
> - why does accept(T) not merit an ISE default?
> 

Notice that for the primitive values the corresponding accept is re-abstracted. 
So Sink accepts references, Sink.OfInt accepts ints etc. so the important 
method to implement remains abstract.


> - ChainedReference/ChainedInt/etc : Should mention in the first sentence that 
> they are abstract. "Abstract skeleton {@code Sink} implementation for 
> creating chains of sinks."
> 
> 
> Tripwire::
> 
> - "Turned off for production." -> "Normally this should be turned off for 
> production systems."
> 
> 
> Node::
> 
> - I don't find the defaults particularly helpful. I would probably prefer 
> them to be fully abstract so that I remember to implement them.

I have found them helpful since the majority of Node implementations are leaf 
nodes.


> 
> - ::forEach() - worth mentioning that the traversal order is possibly 
> unspecified?
> 

Its a bit like that for Collection. We could say something like "in the order 
elements are returned by a spliterator".


> - ::toArray() warning regarding shared array and mutation is not sufficiently 
> dire.
> 

Suggestions?


> - ::toArray() generator is unexplained. relation between generator needs to 
> be clear. ie. generator might not be called even for unshared.
> 

OK. A copy should always be made, using and returning the generated array, if a 
reference cannot be returned.


> - ::copyInto() doesn't offer the ability to get some subset of node elements. 
> count() returns long but arrays can only hold INTEGER.MAX_VALUE  (less 
> actuall

Re: Review Request: JDK-8001334 - Remove use of JVM_* functions from java.io code

[Alan Bateman]

On 11/03/2013 23:43, Dan Xu wrote:
Thanks for all your comments. I have updated the fix accordingly. 
Please see the webrev at 
http://cr.openjdk.java.net/~dxu/8001334/webrev.02/.


For the language concern in getLastErrorString(char *buf, size_t len) 
function, I will log another bug and address it later. Thanks!


-Dan
You've addressed all my comments but I think I may have confused you on 
one point when I mentioned O_APPEND. You've changed handleAppend to use 
fcntl(F_GETFL) and check if the flag is set but this will happen on 
every write in append mode and we don't want that. I think you can 
simply change IO_Append to be handleWrite or else have handleAppend call 
handleWrite. The jboolean flag isn't needed for the *nix case.


-Alan.

hg: jdk8/tl/jdk: 7154889: Non-zero padding is still not allowed in the tableswitch/lookupswitch instructions.

[coleen . phillimore]

Changeset: 334ddf3b101f
Author:coleenp
Date:  2013-03-12 10:35 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/334ddf3b101f

7154889: Non-zero padding is still not allowed in the tableswitch/lookupswitch 
instructions.
Summary: Do not check that the padding bytes are zero if class file format 
version >=51.
Reviewed-by: dholmes, coleenp, mullan, kvn
Contributed-by: harold.sei...@oracle.com

! src/share/native/common/check_code.c

Re: Code review request: JDK-8008670 (partial java.util.stream implementation)

[Paul Sandoz]

On Mar 12, 2013, at 2:29 AM, Mike Duigou  wrote:

> Some notes:
> 
> - Copyrights update.
> 

Pushed to lambda repo:

  http://hg.openjdk.java.net/lambda/lambda/jdk/rev/40c0a71455d7


> - The order of notes on files is the order that I read the files. It seems 
> like not a bad order to review them.
> 
> - package docs? Coming next?
> 

Soonish :-) there is much left to review/spec.


> java.util.Map::
> 
> - @since 1.8 missing for defaults
> 
> -  should be replace with {@code}
> 

It is mostly consistent with the rest of the Map documentation. We should do a 
global replace in that case?


> - default methods need the notice that the default implementations do not 
> provide atomicity. (copy from putIfAbsent)
> 

I have shuffled stuff around and used @implSpec.


> - ::forEach() - Rename the parameter from block?
> 

I updated that to use similar language as Iterator/Iterable.forEach.


> - ::computeIfAbsent() - "and enters it into this map" -> inserts?
> 
> - I wonder if compute if absent should replace the original get(key) with 
> containsKey(key) and replace get() with containsKey() in the pseudo-code 
> documentation. If implementations know that the map can't contain null then 
> they can optimize to get(). Demonstrated with get() would seem to produce 
> incorrect results with a present null value.
> 

> - ::computeIfAbsent()/computeIfPresent()/compute() - "key with which the 
> specified value is to be associated" -> derived value.
> 

Perhaps the above three merit a separate discussion. Raise in a separate email 
thread?


> - ::computeIfAbsent()/computeIfPresent()/compute() - @throws RuntimeException 
> or Error These should be separate.
> 

Do we really need to declare that? It would apply for every use of  lambdas and 
it is previously stated:

"If the function itself throws an (unchecked) exception, 
the exception is rethrown, and the current mapping is
left unchanged."

I have removed them.

http://hg.openjdk.java.net/lambda/lambda/jdk/rev/36664688aa52


> java.util.Collections::
> 
> - Map.synchronizedMap needs extension to provide synchronized implementations 
> of Map default methods.
> 

http://hg.openjdk.java.net/lambda/lambda/jdk/rev/077efaf92c8c

You know off any tests that can be reused?


> java.util.Hashtable::
> 
> - synchronized implementations of Map default methods are needed.
> 

Good point.  I think it may help to address that and some other things 
together. Checked and unmodifiable lists should also be updated to be efficient 
for appropriate default methods.

There is also the synchronized collections/lists. We need to consider the 
spliterator/stream/parallelStream methods, especially how the spliterator 
should behave.

Paul.

Re: Request for review- RFE 8005716

[Alan Bateman]

On 11/03/2013 15:37, BILL PITTORE wrote:

On 3/11/2013 9:40 AM, Alan Bateman wrote:

On 08/03/2013 02:22, BILL PITTORE wrote:
Moved the string allocation into buildJniFunctionName as Alan 
suggested. Built and tested on windows and linux. Updated the webrev:


http://cr.openjdk.java.net/~bpittore/8005716/jdk-webrev.02/


bill
I see this updates the method descriptions to take on board Jeremy's 
comment on the possibility of the library being statically linked 
with the main executable with or without the VM. To be complete, I 
think this will require an update to the UnstatisfiedLinkError 
description too.


Thanks for moving the sizing/allocation of the function name into 
buildJniFunctionName as that is cleaner and safer. Is FILENAME_MAX 
really the right limit to impose?

Maybe JVM_MAXPATHLEN is the better choice.
As it's a symbol rather than a file path then it's probably not right 
either. I guess the question is whether a limit needs to be checked here 
or not.


-Alan.

Re: Code review request: JDK-8008670 (partial java.util.stream implementation)

[Paul Sandoz]

On Mar 12, 2013, at 12:13 AM, Paul Benedict  wrote:
> 3) StreamShape: Javadoc should be formatted to be within 80 characters.
> 

Hmm... that is linked to an old version.

It should be the same as:

http://hg.openjdk.java.net/lambda/lambda/jdk/raw-file/9ccdccc3c754/src/share/classes/java/util/stream/StreamShape.java

Paul.

hg: jdk8/tl/langtools: 8005205: tests missing bugid for repeating annotation change

[joel . franck]

Changeset: 7fe9b9d29095
Author:jfranck
Date:  2013-03-12 11:16 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/langtools/rev/7fe9b9d29095

8005205: tests missing bugid for repeating annotation change
Reviewed-by: jjg, ssides

! test/tools/javac/annotations/repeatingAnnotations/MissingContainer.java
! test/tools/javac/annotations/repeatingAnnotations/MissingDefaultCase1.java

Re: RFR: 8009428 and 8009429 - Profile related fixes and clean ups

[Alan Bateman]

On 12/03/2013 07:10, David Holmes wrote:

:


For the intro comment in profile-rtjar-includes.txt then it might be
useful to beef up the comment to explain what happens when an API
package does not match one of the rules, ie: does it go into compact1,
only the full JRE, or none. Also make it explicit that the form
DialogCallbackHandler*.class should not be used. I suggest this for the
benefit of someone needing to tweak this in the future.


I have updated the webrev with additional commentary.

Thanks, that will be very useful to future maintainers.




I have played with com.sun.tools.javac.sym.Profiles and so the changes
to MakefileProfiles look okay to me but Jon should really be the
reviewer for this. One thing about maxProfiles is that it should match
Profile.values.length maybe maxProfiles should not be hardcoded to 4.


Sorry but what is Profiles.values.length? Previously we inferred the 
number of profiles from the fact that we failed to find PROFILE_n for 
some value n. That can't work (easily) now hence the hard limit.
I meant com.sun.tools.javac.jvm.Profile, it's an enum of the profiles so 
it means that the knowledge about 3 + full JRE is now in two places.






Another thing is whether to add a test or beef up an existing test
(ProfileOptionTest.java in particular).


What exactly is it that you would like to test for?
I think the test should include a few cases to cover a few selected 
types in sub-packages to make sure they are in the right profile.


-Alan.

Re: Code review request: JDK-8008670 (partial java.util.stream implementation)

[Peter Levart]

On 03/12/2013 02:29 AM, Mike Duigou wrote:

Some notes:

- Copyrights update.

- The order of notes on files is the order that I read the files. It seems like 
not a bad order to review them.

- package docs? Coming next?

java.util.Map::

- @since 1.8 missing for defaults

-  should be replace with {@code}

- default methods need the notice that the default implementations do not 
provide atomicity. (copy from putIfAbsent)

- ::forEach() - Rename the parameter from block?

- ::computeIfAbsent() - "and enters it into this map" -> inserts?

- I wonder if compute if absent should replace the original get(key) with 
containsKey(key) and replace get() with containsKey() in the pseudo-code 
documentation. If implementations know that the map can't contain null then 
they can optimize to get(). Demonstrated with get() would seem to produce 
incorrect results with a present null value.

- ::computeIfAbsent()/computeIfPresent()/compute() - "key with which the specified 
value is to be associated" -> derived value.

- ::computeIfAbsent()/computeIfPresent()/compute() - @throws RuntimeException 
or Error These should be separate.


Hi,

I would just like to point out to a discussion a couple of months ago:

http://mail.openjdk.java.net/pipermail/lambda-dev/2013-January/007509.html

... at least Map.compute() still has this anomaly when executed against 
HashMap.


Regards, Peter



java.util.Collections::

- Map.synchronizedMap needs extension to provide synchronized implementations 
of Map default methods.

java.util.Hashtable::

- synchronized implementations of Map default methods are needed.


AbstractTask::

- "that describes a portion of the input" -> describes the he portion of the 
input associated with the subtree rooted at this task.

- setRawResult() : needs better docs. Why does getRawResult return local (which 
may be null)?

- Are trees of abstract task always homogeneous? Perhaps an assert 
parent.getClass == class ? since the T can't be verified to be the same as the 
class.

- onCompletion does not clear numChildren



ForEachOps::

- BooleanProvider -> BooleanSupplier

- in whatever Thread -> on whatever thread

- "There is no guarantee that additional elements.." should be before 
@apiNote.

- Wouldn't it be nice if there as a way to template the primitive impls. :-)


IntermediateOp::

- "An intermediate operation has an input type and an output type". These 
aren't reified or introspectable in contrast to the shape.

- Perhaps defer more about statefulness to StatefulOp


StatefulOp::

- Should include a sentence about how a stateful op knows when it has received 
it's last element.

- Perhaps re-iterate wrapSink() to provide documentation about completion and 
when elements are written to output. ie. Sink.end()


Sink::

- ::end() "If the {@code Sink} buffers any results from previous values, they should 
dump their contents downstream and clear any stored state." -> If the {@code Sink} 
is stateful it should send all of it's stored state downstream.

- :: I wonder about the suggestion to clear stored state. Why is this better 
than just letting it be GCed? If there is value to explicit clearing we should 
explain why (at least partially).

- ::cancellationRequested() -> :: accepting()? Cancellation sounds like an 
exceptional state.

- ::cancellationRequested() - document default. @implNote

- why does accept(T) not merit an ISE default?

- ChainedReference/ChainedInt/etc : Should mention in the first sentence that they are 
abstract. "Abstract skeleton {@code Sink} implementation for creating chains of 
sinks."


Tripwire::

- "Turned off for production." -> "Normally this should be turned off for production 
systems."


Node::

- I don't find the defaults particularly helpful. I would probably prefer them 
to be fully abstract so that I remember to implement them.

- ::forEach() - worth mentioning that the traversal order is possibly 
unspecified?

- ::toArray() warning regarding shared array and mutation is not sufficiently 
dire.

- ::toArray() generator is unexplained. relation between generator needs to be 
clear. ie. generator might not be called even for unshared.

- ::copyInto() doesn't offer the ability to get some subset of node elements. 
count() returns long but arrays can only hold INTEGER.MAX_VALUE  (less 
actually) elements. Either there has to be a way to get partial results or we 
might as well make count an int.

- ::Builder - is it necessary to say that it is a flat node?

- That's a lotta template code!



StreamOpFlag::

- tables could be a proper HTML table. If you want me to rework it as a proper 
508 compliant table, just ask. :-)


On Mar 11 2013, at 14:55 , Brian Goetz wrote:


I've posted an updated webrev incorporating comments received to date:

  http://cr.openjdk.java.net/~briangoetz/jdk-8008670.2/webrev/



On 2/21/2013 2:16 PM, Brian Goetz wrote:

At:
   http://cr.openjdk.java.net/~briangoetz/jdk-8008670/webrev/

there is a webrev of a subset of the implementa

Re: RFR: 8009428 and 8009429 - Profile related fixes and clean ups

[David Holmes]

Hi Alan,

On 11/03/2013 6:54 PM, Alan Bateman wrote:

On 11/03/2013 01:24, David Holmes wrote:

I had overlooked the need to update the ct.sym creation tool to
recognize the new syntax in the profile spec file. That process also
uncovered a few bugs in the listing that needed correcting.

The javadoc generation of compact profile information is not quite
right, but that will be handled in a seperate bug.

Updated webrevs at:

http://cr.openjdk.java.net/~dholmes/8009428_8009429.v2/

The changes to the top-level and jdk repo look good to me. As per my
original comments, I'm happy to see profile-rtjar-includes.txt changed
to be much easier to maintain. The assumption that jdk.** is in compact1
is okay for now but I'm sure that will need to be changed when that
namespace is used more.


Yes we will change this as we need to, once other packages come into play.


For the intro comment in profile-rtjar-includes.txt then it might be
useful to beef up the comment to explain what happens when an API
package does not match one of the rules, ie: does it go into compact1,
only the full JRE, or none. Also make it explicit that the form
DialogCallbackHandler*.class should not be used. I suggest this for the
benefit of someone needing to tweak this in the future.


I have updated the webrev with additional commentary.


I have played with com.sun.tools.javac.sym.Profiles and so the changes
to MakefileProfiles look okay to me but Jon should really be the
reviewer for this. One thing about maxProfiles is that it should match
Profile.values.length maybe maxProfiles should not be hardcoded to 4.


Sorry but what is Profiles.values.length? Previously we inferred the 
number of profiles from the fact that we failed to find PROFILE_n for 
some value n. That can't work (easily) now hence the hard limit.



Another thing is whether to add a test or beef up an existing test
(ProfileOptionTest.java in particular).


What exactly is it that you would like to test for?

Thanks,
David


-Alan.