Re: RFR: 8273826: Correct Manifest file name and NPE checks [v2]

2021-10-07 Thread Sean Mullan 
On Thu, 7 Oct 2021 15:09:22 GMT, Sean Coffey  wrote:

>> test/jdk/sun/security/tools/jarsigner/warnings/LowerCaseManifest.java line 
>> 38:
>> 
>>> 36:  * @library /test/lib ../
>>> 37:  * @build jdk.test.lib.util.JarUtils
>>> 38:  * @run main LowerCaseManifest
>> 
>> You don't need this line as this is the default @run setting.
>
> Looks like it's required if there's a @build directive

Ok.

-

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

Re: RFR: 8273826: Correct Manifest file name and NPE checks [v2]

2021-10-07 Thread Weijun Wang 
On Thu, 7 Oct 2021 15:12:33 GMT, Sean Mullan  wrote:

>> Looks like it's required if there's a @build directive
>
> Ok.

It might be necessary because there is already a `@build` there.

-

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

Re: RFR: 8273826: Correct Manifest file name and NPE checks [v2]

2021-10-07 Thread Sean Coffey 
On Thu, 7 Oct 2021 14:57:53 GMT, Sean Mullan  wrote:

>> Sean Coffey has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Make variables final
>
> test/jdk/sun/security/tools/jarsigner/warnings/LowerCaseManifest.java line 38:
> 
>> 36:  * @library /test/lib ../
>> 37:  * @build jdk.test.lib.util.JarUtils
>> 38:  * @run main LowerCaseManifest
> 
> You don't need this line as this is the default @run setting.

Looks like it's required if there's a @build directive

-

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

Re: RFR: 8273826: Correct Manifest file name and NPE checks [v2]

2021-10-07 Thread Weijun Wang 
On Thu, 7 Oct 2021 14:21:27 GMT, Sean Coffey  wrote:

>> src/java.base/share/classes/sun/security/util/ManifestEntryVerifier.java 
>> line 66:
>> 
>>> 64: 
>>> 65: private String name = null;
>>> 66: private String manifestFileName;
>> 
>> Make this final and add a comment that it will never be null.
>
> Thanks for the review - Made final and added comment. Also decided to make 
> the "Manifest man" variable final here also while here.

Great, the more final there are, the less I need to look back and forth while 
reading the code.

-

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

Re: RFR: 8273826: Correct Manifest file name and NPE checks [v2]

2021-10-07 Thread Sean Mullan 
On Thu, 7 Oct 2021 10:44:39 GMT, Sean Coffey  wrote:

>> Use correct manifest file name in the Manifest verifier checks. 
>> Also - extra null check
>> 
>> The test doesn't reproduce the exact issue reported but should prevent 
>> future regressions in this area.
>
> Sean Coffey has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Make variables final

Marked as reviewed by mullan (Reviewer).

test/jdk/sun/security/tools/jarsigner/warnings/LowerCaseManifest.java line 38:

> 36:  * @library /test/lib ../
> 37:  * @build jdk.test.lib.util.JarUtils
> 38:  * @run main LowerCaseManifest

You don't need this line as this is the default @run setting.

-

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

Re: RFR: 8273826: Correct Manifest file name and NPE checks [v2]

2021-10-07 Thread Sean Coffey 
On Wed, 6 Oct 2021 17:54:15 GMT, Weijun Wang  wrote:

>> Sean Coffey has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Make variables final
>
> src/java.base/share/classes/sun/security/util/ManifestEntryVerifier.java line 
> 66:
> 
>> 64: 
>> 65: private String name = null;
>> 66: private String manifestFileName;
> 
> Make this final and add a comment that it will never be null.

Thanks for the review - Made final and added comment. Also decided to make the 
"Manifest man" variable final here also while here.

-

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

Re: RFR: 8273826: Correct Manifest file name and NPE checks [v2]

2021-10-07 Thread Sean Coffey 
> Use correct manifest file name in the Manifest verifier checks. 
> Also - extra null check
> 
> The test doesn't reproduce the exact issue reported but should prevent future 
> regressions in this area.

Sean Coffey has updated the pull request incrementally with one additional 
commit since the last revision:

  Make variables final

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/5841/files
  - new: https://git.openjdk.java.net/jdk/pull/5841/files/a63c81ad..3d72a6d7

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

  Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5841.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5841/head:pull/5841

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