[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-compiler-dev
Subject: Re: RFR: 8201274: Launch Single-File Source-Code Programs
From: Jonathan Gibbons <jonathan.gibbons () oracle ! com>
Date: 2018-04-25 17:58:31
Message-ID: 5AE0C1C7.5010300 () oracle ! com
[Download RAW message or body]
Kumar,
Thank you for your feedback; I will incorporate it in the next webrev.
-- Jon
On 04/25/2018 09:38 AM, Kumar Srinivasan wrote:
> Hi John,
>
> I focused mainly on the native side, looks ok, except for a couple of
> minor issues.
>
> java.c
> 1320 const char *prop = "-Djdk.internal.javac.source=";
> 1321 size_t size = JLI_StrLen(prop) + JLI_StrLen(value) + 1;
> 1322 char *propValue = (char *)JLI_MemAlloc(size + 1);
>
> I think we are allocating extra byte ^^^^^^^
>
> 1323 JLI_StrCpy(propValue, prop);
> 1324 JLI_StrCat(propValue, value);
>
> I think we can do this, safer and neater, as follows:
>
> size_t size = JLI_StrLen(prop) + JLI_StrLen(value);
> char *propValue = (char *)JLI_MemAlloc(size + 1);
> JLI_Snprintf(propValue, size, "%s%s", prop, value);
> 1483 if (mode == LM_SOURCE) {
> 1484 AddOption("--add-modules=ALL-DEFAULT", NULL);
> 1485 *pwhat = SOURCE_LAUNCHER_MAIN_ENTRY;
> 1486 *pargc = argc + 1;
> 1487 *pargv = argv - 1;
>
> A short comment perhaps ? why we are incrementing argc, and
> decrementing argv,
> saves some head scratching for a casual reader.
>
> I looked at the launcher tests, very nice.
>
>
> Thanks
> Kumar
>
>
>
>
>
> On 4/12/2018 1:15 PM, 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
>
[Attachment #3 (text/html)]
<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Kumar,<br>
<br>
Thank you for your feedback; I will incorporate it in the next
webrev.<br>
<br>
-- Jon<br>
<br>
<div class="moz-cite-prefix">On 04/25/2018 09:38 AM, Kumar
Srinivasan wrote:<br>
</div>
<blockquote cite="mid:5AE0AF08.10105@oracle.com" type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
Hi John,<br>
<br>
I focused mainly on the native side, looks ok, except for a
couple of <br>
minor issues.<br>
<br>
java.c<br>
<pre><span class="new">1320 const char *prop = \
"-Djdk.internal.javac.source=";</span> <span class="new">1321 size_t \
size = JLI_StrLen(prop) + JLI_StrLen(value) + 1;</span> <span class="new">1322 \
char *propValue = (char *)JLI_MemAlloc(size + 1);</span>
I think we are allocating extra byte ^^^^^^^
<span class="new">1323 JLI_StrCpy(propValue, prop);</span>
<span class="new">1324 JLI_StrCat(propValue, value);
I think we can do this, safer and neater, as follows:
</span><span class="new">size_t size = JLI_StrLen(prop) + JLI_StrLen(value);</span>
<span class="new">char *propValue = (char *)JLI_MemAlloc(size + 1);</span>
JLI_Snprintf(propValue, size, "%s%s", prop, value);
</pre>
<pre><span class="new">1483 if (mode == LM_SOURCE) {</span>
<span class="new">1484 AddOption("--add-modules=ALL-DEFAULT", NULL);</span>
<span class="new">1485 *pwhat = SOURCE_LAUNCHER_MAIN_ENTRY;</span>
<span class="new">1486 *pargc = argc + 1;</span>
<span class="new">1487 *pargv = argv - 1;
</span></pre>
<span class="new">A short comment perhaps ? why we are
incrementing argc, and decrementing argv,</span><br>
<span class="new">saves some head scratching for a casual reader.</span><br>
<span class="new"></span><br>
<span class="new">I looked at the launcher tests, very nice.</span><br>
<span class="new"></span><br>
<span class="new"></span><br>
<span class="new">Thanks</span><br>
<span class="new">Kumar</span><br>
<span class="new"></span><span class="new"></span><br>
<span class="new"></span><span class="new"><br>
</span><br>
<br>
<br>
<div class="moz-cite-prefix">On 4/12/2018 1:15 PM, Jonathan
Gibbons wrote:<br>
</div>
<blockquote cite="mid:5ACFBE5C.1080508@oracle.com" type="cite">
<meta http-equiv="content-type" content="text/html;
charset=utf-8">
Please review an initial implementation for the feature
described in <br>
JEP 330: Launch Single-File Source-Code Programs.<br>
<br>
The work is described in the JEP and CSR, and falls into various
parts:<br>
<ul>
<li>The part to handle the new command-line options is in the
native Java launcher code.</li>
<li>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.</li>
<li>There are some minor Makefile changes, to add support for
a new resource file.</li>
</ul>
There are no changes to javac itself.<br>
<br>
JEP: <a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://openjdk.java.net/jeps/330">http://openjdk.java.net/jeps/330</a><br>
JBS: <a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8201274">https://bugs.openjdk.java.net/browse/JDK-8201274</a><br>
CSR: <a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8201275">https://bugs.openjdk.java.net/browse/JDK-8201275</a><br>
Webrev: <a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejjg/8201274/webrev.00/">http://cr.openjdk.java.net/~jjg/8201274/webrev.00/</a><br>
<br>
-- Jon<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic