RFR: 8201274: Launch Single-File Source-Code Programs

[Maurizio Cimadamore]

One small followup question - the test SourceLauncherTest is not 
covering any shebang cases - is that deliberate? I see that those seem 
to be covered in the launcher test too, but I wonder if we should have 
tests for clearly broken stuff, such as

'#'

'#!'

'#!\n'

Another small question - I see that the shebang process essentially 
replaces the first line separator with \n - that is probably ok given 
that the file is processed internally after that - but I wonder if you 
have thought about pros and cons of preserving the original line separator.

Cheers
Maurizio


On 12/04/18 21:46, Maurizio Cimadamore wrote:
> Looks great - some initial comments (I can't really comment on the 
> launcher changes):
>
> * This logic is efficient:
>
> int magic = (in.read() << 8) + in.read();
> boolean shebang = magic == (('#' << 8) + '!');
>
> but convoluted to read; perhaps could be improved slightly by making 
> '#' << 8) + '!' a static constant, and comparing against that.
>
> * I note that the reading logic in general could be simplified, e.g. 
> using Files.lines(Path) - but I assume that you wrote the code as is 
> for performance reasons (e.g. to avoid creating too many string 
> objects) ?
>
> * I see that both the file manager and the class loader reasonably 
> share the same context: a Map<String, byte[]>. I would make this more 
> explicit, by having a Context class, whose state is the map, and then 
> have the context provide two methods:
>
> getClassLoader()
> getFileManager()
>
> This way the sharing will be automatic, no need to extract one field 
> from one place and pass it over to the other place.
>
> * Big whohoo for being able to use the enhanced diagnostic framework 
> with a couple of tweaks on the makefile - I hope that would have been 
> the case when I put in the support, but since we have never done it - 
> wasn't 100% sure it would work ;-)
>
>
> Overall I like it quite a lot and I think you went for a really clean 
> design - kudos!
>
> Maurizio
>
>
>
> On 12/04/18 21:15, Jonathan Gibbons wrote:
>> Please review an initial implementation for the feature described in
>> JEP 330: Launch Single-File Source-Code Programs.
>>
>> The work is described in the JEP and CSR, and falls into various parts:
>>
>>   * The part to handle the new command-line options is in the native
>>     Java launcher code.
>>   * The part to invoke the compiler and subsequently execute the code
>>     found in the source file is in a new class in the jdk.compiler 
>> module.
>>   * There are some minor Makefile changes, to add support for a new
>>     resource file.
>>
>> There are no changes to javac itself.
>>
>> JEP: http://openjdk.java.net/jeps/330
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8201274
>> CSR: https://bugs.openjdk.java.net/browse/JDK-8201275
>> Webrev: http://cr.openjdk.java.net/~jjg/8201274/webrev.00/
>>
>> -- Jon
>