RFR: JDK-8169505 - Update changes by JDK-8159393 to reflect CCC review

[Jim Laskey (Oracle)]

http://cr.openjdk.java.net/~jlaskey/8169505/webrev/index.html
https://bugs.openjdk.java.net/browse/JDK-8169505

Re: [9] RFR: 8167240: Write new tests to cover functionality of existing 'jimage' options

[Jim Laskey (Oracle)]

+1

Really nice, thank you.

> On Nov 15, 2016, at 11:16 AM, Denis Kononenko  
> wrote:
> 
> 
> Hi,
> 
> Please do re-review for these changes.
> 
> 1) tests for list --include were rewritten accordingly to 
> https://bugs.openjdk.java.net/browse/JDK-8167384;
> 2) removed tests for '@filename', see 
> https://bugs.openjdk.java.net/browse/JDK-8169720;
> 3) two new CRs were submitted: 
> https://bugs.openjdk.java.net/browse/JDK-8169715, 
> https://bugs.openjdk.java.net/browse/JDK-8169713;
> 
> WEBREV: http://cr.openjdk.java.net/~dkononenko/8167240/webrev.04/
> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8167240
> 
> Thank you,
> Denis.
> 
> 
>> Hi Andrey,
>> 
>> No, it isn't. I submitted a new CR:
>> https://bugs.openjdk.java.net/browse/JDK-8167384.
>> 
>> Thank you,
>> Denis.
>> 
>>> Hi,
>>> 
>>> Looks OK.  Is it 100% pass rate?
>>> 
>>> —Andrey
 On 9 Nov 2016, at 20:36, Denis Kononenko
>>>  wrote:
 
 
 
 Hi,
 
 After discussion with Andrey we decided to add more tests for
>> corner
>>> cases.
 The new changes are available by the link below.
 
 WEBREV: http://cr.openjdk.java.net/~dkononenko/8167240/webrev.03/
 BUGURL: https://bugs.openjdk.java.net/browse/JDK-8167240
 
 
 Thank you,
 Denis.
 
> Hi,
> 
> Looks OK to me.
> I can suggest two more cases. A directory and file symlink can
>> be
> passed in options where tool requires a file path. 
> 
> —Andrey
> 
>> On 8 Nov 2016, at 16:17, Denis Kononenko
>  wrote:
>> 
>> 
>> Hi,
>> 
>> The new version of changes.
>> 
>> - Switched back to jdk/test/testlibrary to avoid unwanted
> dependencies (JImageToolTest.java);
>> - Verified tests on smallest possible JDK build.
>> 
>> WEBREV:
>> http://cr.openjdk.java.net/~dkononenko/8167240/webrev.02/
>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8167240
>> 
>> Thank you,
>> Denis.
>> 
>>> Denis,
>>> 
>>> I can see that you have switched to the top level test library
> with
>>> this change. With that you are getting more module
>> dependencies
> than 
>>> just java.base. First of all, it would probably make sense to
> build
>>> only the classes you needed (which would be 
>>> jdk.test.lib.process.ProcessTools, I assume), but even if you
>>> only
>>> build that, jdk.test.lib.process.ProcessTools has dependencies
> outside
>>> java.base module.
>>> 
>>> You either have to declare @modules in your test or go back to
>>> the
>>> jdk/test/lib/testlibrary. Then, of course, unneeded module
>>> dependencies are questionable.
>>> 
>>> Shura
>>> 
>>> 
 On Nov 3, 2016, at 6:29 AM, Denis Kononenko
>>>  wrote:
 
 Hi,
 
 I've done some rework accordingly to Alan's and Shura's
>>> comments:
 
 1) removed overlapped tests from JImageToolTest.java;
 
 2) added new tests JImageVerifyTest.java for jimage verify;
 
 3) reorganized jtreg's tags;
 
 The new WEBREV can be found here:
>>> http://cr.openjdk.java.net/~dkononenko/8167240/webrev.01/
 
 Thank you,
 Denis.
 
 On 06.10.2016 19:37, Denis Kononenko wrote:
> Hi,
> 
> Could someone please review these new tests for jimage
>>> utility.
> 
> There're 5 new files containing tests to cover use cases for
>>> 'info', 'list', 'extract' and other options. No new tests for
>>> 'verify'.
> 
> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8167240
> WEBREV:
> http://cr.openjdk.java.net/~dkononenko/8167240/webrev.00/
> 
> 
> Thank you,
> Denis.
 

Re: [9] RFR: 8167240: Write new tests to cover functionality of existing 'jimage' options

[Denis Kononenko]

Hi,

Please do re-review for these changes.

1) tests for list --include were rewritten accordingly to 
https://bugs.openjdk.java.net/browse/JDK-8167384;
2) removed tests for '@filename', see 
https://bugs.openjdk.java.net/browse/JDK-8169720;
3) two new CRs were submitted: 
https://bugs.openjdk.java.net/browse/JDK-8169715, 
https://bugs.openjdk.java.net/browse/JDK-8169713;

WEBREV: http://cr.openjdk.java.net/~dkononenko/8167240/webrev.04/
BUGURL: https://bugs.openjdk.java.net/browse/JDK-8167240

Thank you,
Denis.


> Hi Andrey,
> 
> No, it isn't. I submitted a new CR:
> https://bugs.openjdk.java.net/browse/JDK-8167384.
> 
> Thank you,
> Denis.
> 
> > Hi,
> > 
> > Looks OK.  Is it 100% pass rate?
> > 
> > —Andrey
> > > On 9 Nov 2016, at 20:36, Denis Kononenko
> >  wrote:
> > > 
> > > 
> > > 
> > > Hi,
> > > 
> > > After discussion with Andrey we decided to add more tests for
> corner
> > cases.
> > > The new changes are available by the link below.
> > > 
> > > WEBREV: http://cr.openjdk.java.net/~dkononenko/8167240/webrev.03/
> > > BUGURL: https://bugs.openjdk.java.net/browse/JDK-8167240
> > > 
> > > 
> > > Thank you,
> > > Denis.
> > > 
> > >> Hi,
> > >> 
> > >> Looks OK to me.
> > >> I can suggest two more cases. A directory and file symlink can
> be
> > >> passed in options where tool requires a file path. 
> > >> 
> > >> —Andrey
> > >> 
> > >>> On 8 Nov 2016, at 16:17, Denis Kononenko
> > >>  wrote:
> > >>> 
> > >>> 
> > >>> Hi,
> > >>> 
> > >>> The new version of changes.
> > >>> 
> > >>> - Switched back to jdk/test/testlibrary to avoid unwanted
> > >> dependencies (JImageToolTest.java);
> > >>> - Verified tests on smallest possible JDK build.
> > >>> 
> > >>> WEBREV:
> http://cr.openjdk.java.net/~dkononenko/8167240/webrev.02/
> > >>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8167240
> > >>> 
> > >>> Thank you,
> > >>> Denis.
> > >>> 
> >  Denis,
> >  
> >  I can see that you have switched to the top level test library
> > >> with
> >  this change. With that you are getting more module
> dependencies
> > >> than 
> >  just java.base. First of all, it would probably make sense to
> > >> build
> >  only the classes you needed (which would be 
> >  jdk.test.lib.process.ProcessTools, I assume), but even if you
> > only
> >  build that, jdk.test.lib.process.ProcessTools has dependencies
> > >> outside
> >  java.base module.
> >  
> >  You either have to declare @modules in your test or go back to
> > the
> >  jdk/test/lib/testlibrary. Then, of course, unneeded module
> >  dependencies are questionable.
> >  
> >  Shura
> >  
> >  
> > > On Nov 3, 2016, at 6:29 AM, Denis Kononenko
> >   wrote:
> > > 
> > > Hi,
> > > 
> > > I've done some rework accordingly to Alan's and Shura's
> > comments:
> > > 
> > > 1) removed overlapped tests from JImageToolTest.java;
> > > 
> > > 2) added new tests JImageVerifyTest.java for jimage verify;
> > > 
> > > 3) reorganized jtreg's tags;
> > > 
> > > The new WEBREV can be found here:
> >  http://cr.openjdk.java.net/~dkononenko/8167240/webrev.01/
> > > 
> > > Thank you,
> > > Denis.
> > > 
> > > On 06.10.2016 19:37, Denis Kononenko wrote:
> > >> Hi,
> > >> 
> > >> Could someone please review these new tests for jimage
> > utility.
> > >> 
> > >> There're 5 new files containing tests to cover use cases for
> >  'info', 'list', 'extract' and other options. No new tests for
> >  'verify'.
> > >> 
> > >> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8167240
> > >> WEBREV:
> > >> http://cr.openjdk.java.net/~dkononenko/8167240/webrev.00/
> > >> 
> > >> 
> > >> Thank you,
> > >> Denis.
> > >

Re: RFR: JDK-8169720 - jimage help message for --include option should be corrected

[Sundararajan Athijegannathan]

+1


On 11/15/2016 8:09 PM, Jim Laskey (Oracle) wrote:
> http://cr.openjdk.java.net/~jlaskey/8169720/webrev/index.html 
> 
> https://bugs.openjdk.java.net/browse/JDK-8169720 
> 
>

Re: RFR: JDK-8169720 - jimage help message for --include option should be corrected

[Alan Bateman]

On 15/11/2016 14:39, Jim Laskey (Oracle) wrote:

http://cr.openjdk.java.net/~jlaskey/8169720/webrev/index.html 

https://bugs.openjdk.java.net/browse/JDK-8169720 



Looks fine.

RFR: JDK-8169720 - jimage help message for --include option should be corrected

[Jim Laskey (Oracle)]

http://cr.openjdk.java.net/~jlaskey/8169720/webrev/index.html 

https://bugs.openjdk.java.net/browse/JDK-8169720 

Re: Compiling Java 9

[Stephan Herrmann]

Hi Alex,


On 11/15/2016 12:20 AM, Alex Buckley wrote:

You have a lot of questions here, mainly not about the OpenJDK implementation 
of JSR 379 but rather about JSR 379 itself. I'm
working on JLS text that will be presented alongside lang-vm.html in the next 
few weeks, but here are some quick points:


I'm looking forward to the JLS text very much ...


- lang-vm.html does not say that restricted keywords are keywords only inside a 
module declaration; it says they are keywords
_solely where they appear as terminals in ModuleDeclaration_.


Thanks, so for one this settles, that the compilation unit is irrelevant for 
the questions
at hand, only the syntactical structure of the content is relevant, right?

I'm still reluctant to believe the full consequences of what you seem to be 
saying.
With ModuleDeclaration being a non-terminal in the grammar, is the definition of
"restricted keywords" intended to mean:
   Whether or not a word is a keyword or identifier is decided *after* parsing 
has completed?
   Viz.: if a given text could be parsed as a ModuleDeclaration then exactly 
those words
   that have been used as keywords are keywords.
It seems so, because before parsing we don't have any knowledge whether or not 
a given
text *is* a ModuleDeclaration.
Is it even justified to speak of "parsing" in this context? Syntax inference?

The current grammar can be parsed, e.g., with a hand-written scannerless parser,
but can we assume any regularities about the grammar now and in the future?
If not, should all tool implementers be prepared to replace the parser with
full pattern matching with back tracking, as to try all possible combinations of
interpreting keyword candidates as keywords or identifiers?

Thinking aloud about possible consequences, I wonder what happens in case of a 
syntax error.
Strictly speaking a text that almost looks like a ModuleDeclaration but still 
cannot be fully
accepted as such, contains no keywords at all (because we have no 
ModuleDeclaration), right?
That's probably a fact which tools should carefully hide from users, to avoid 
producing
utterly confusing error messages. Generally speaking, the only "correct" syntax 
error
message seems to be: "the input text could not be matched as a 
ModuleDeclaration".

Picking on words, I still hold that my example contains restricted keywords as 
terminals
in a ModuleDeclaration where they are apparently interpreted (by javac) as 
identifiers:

module module {// second word is an identifier
requires requires; // second word is an identifier
exports to to exports; // words #2 and #4 are identifiers
uses module;   // second word is an identifier
provides uses with to; // words #2 and #4 are identifiers
}

So, am I possibly still barking up the wrong tree?
Or should the definition really say
   "where they appear as _keywords_ in ModuleDeclaration"
?

thanks,
Stephan