RFR: 8150303: Rewrite test/tools/javac/Paths/Diagnostics.sh

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8150303: Rewrite test/tools/javac/Paths/Diagnostics.sh

Guoxiong Li-2
Hi all,

This patch rewrites Diagnostics.sh to java code by using ToolBox API.
And I have a question: What does the `elts` mean in the following comments? Is it `elements`?
I think that a meaningful name should be used.

// Warn for missing elts in user-specified paths
// No warning for missing elts in "system" paths

Thank you for taking the time to review.

Best Regards.

-------------

Commit messages:
 - 8150303: Rewrite test/tools/javac/Paths/Diagnostics.sh

Changes: https://git.openjdk.java.net/jdk/pull/1959/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1959&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8150303
  Stats: 520 lines in 2 files changed: 310 ins; 210 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1959.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1959/head:pull/1959

PR: https://git.openjdk.java.net/jdk/pull/1959
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8150303: Rewrite test/tools/javac/Paths/Diagnostics.sh

Jonathan Gibbons-2
On Wed, 6 Jan 2021 14:18:49 GMT, Guoxiong Li <[hidden email]> wrote:

> This patch rewrites Diagnostics.sh to java code by using ToolBox API.
> And I have a question: What does the elts mean in the following comments? Is it elements?
> I think that a meaningful name should be used.

Brave person. There's a reason no one has converted this test so far.

To answer your specific question: yes, `elts` should be read as an abbreviation for `elements` and yes, it would be good to clarify it.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1959
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8150303: Rewrite test/tools/javac/Paths/Diagnostics.sh [v2]

Guoxiong Li-2
In reply to this post by Guoxiong Li-2
> Hi all,
>
> This patch rewrites Diagnostics.sh to java code by using ToolBox API.
> And I have a question: What does the `elts` mean in the following comments? Is it `elements`?
> I think that a meaningful name should be used.
>
> // Warn for missing elts in user-specified paths
> // No warning for missing elts in "system" paths
>
> Thank you for taking the time to review.
>
> Best Regards.

Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision:

  Modify elts to elements. Use correct line separator.

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1959/files
  - new: https://git.openjdk.java.net/jdk/pull/1959/files/7f2de0ff..ae0379b6

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1959&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1959&range=00-01

  Stats: 14 lines in 1 file changed: 4 ins; 0 del; 10 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1959.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1959/head:pull/1959

PR: https://git.openjdk.java.net/jdk/pull/1959
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8150303: Rewrite test/tools/javac/Paths/Diagnostics.sh

Guoxiong Li-2
In reply to this post by Jonathan Gibbons-2
On Wed, 6 Jan 2021 14:54:13 GMT, Jonathan Gibbons <[hidden email]> wrote:

>> Hi all,
>>
>> This patch rewrites Diagnostics.sh to java code by using ToolBox API.
>> And I have a question: What does the `elts` mean in the following comments? Is it `elements`?
>> I think that a meaningful name should be used.
>>
>> // Warn for missing elts in user-specified paths
>> // No warning for missing elts in "system" paths
>>
>> Thank you for taking the time to review.
>>
>> Best Regards.
>
>> This patch rewrites Diagnostics.sh to java code by using ToolBox API.
>> And I have a question: What does the elts mean in the following comments? Is it elements?
>> I think that a meaningful name should be used.
>
> Brave person. There's a reason no one has converted this test so far.
>
> To answer your specific question: yes, `elts` should be read as an abbreviation for `elements` and yes, it would be good to clarify it.

@jonathan-gibbons Thank you for your answer. I modify `elts` to `elements`.

And I revise the test code about line separator because the test failed at windows in the pre-submit tests.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1959
Reply | Threaded
Open this post in threaded view
|

Withdrawn: 8150303: Rewrite test/tools/javac/Paths/Diagnostics.sh

duke
In reply to this post by Guoxiong Li-2
On Wed, 6 Jan 2021 14:18:49 GMT, Guoxiong Li <[hidden email]> wrote:

> Hi all,
>
> This patch rewrites Diagnostics.sh to java code by using ToolBox API.
> And I have a question: What does the `elts` mean in the following comments? Is it `elements`?
> I think that a meaningful name should be used.
>
> // Warn for missing elts in user-specified paths
> // No warning for missing elts in "system" paths
>
> Thank you for taking the time to review.
>
> Best Regards.

This pull request has been closed without being integrated.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1959