Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v17]
On Wed, 1 Jun 2022 18:26:17 GMT, Naoto Sato wrote:
>> XenoAmess has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> do it as naotoj said
>
> src/java.base/share/classes/java/util/Calendar.java line 2648:
>
>> 2646: set.add("gregory");
>> 2647: set.add("buddhist");
>> 2648: set.add("japanese");
>
> This can be replaced with `SET = Set.of("gregory", "buddhist", "japanese");`.
@naotoj Yes it can. I did a further clean up to it, please have a look.
-
PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v17]
On Wed, 1 Jun 2022 17:34:04 GMT, Stuart Marks wrote: >> XenoAmess has updated the pull request incrementally with one additional >> commit since the last revision: >> >> do it as naotoj said > > test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 441: > >> 439: } >> 440: } >> 441: > > This unifies the test cases between the Set and Map factories, which > accomplishes the primary goal. Good. > > The unification is achieved through classic object-oriented polymorphism, > which works fine, though which is rather verbose. This could probably be > reduced with some tinkering on the model, but it's probably reaching the > point where additional tinkering on the model isn't worth it. I'm ok with > sticking with this approach for now. Maybe we can clean it up later, or maybe > not -- it's at least fairly straightforward. > > One issue that contributes to the verbosity is the repeated null checking. > The null checking enables the test code to proceed and end up with -1 as the > capacity if there's a null in there somewhere. This will cause the assertion > to fail. This is good in that it will call attention to itself (as opposed to > silently passing or something). However, if the test cases are set up > properly, they should never run into null. If the null checking weren't done, > an unexpected null will throw NPE, which will be caught be the framework and > reported as an error. > > That seems perfectly fine to me, so I'd suggest simply removing the null > checking. That would also reduce the bulkiness of infrastructure. @stuart-marks done. - PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v17]
On Fri, 27 May 2022 18:40:32 GMT, XenoAmess wrote: >> as title. > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > do it as naotoj said Changes to `net` and `http` look good. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v17]
On Fri, 27 May 2022 18:40:32 GMT, XenoAmess wrote: >> as title. > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > do it as naotoj said `java.io` and `java.nio` look all right. - Marked as reviewed by bpb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v17]
On Fri, 27 May 2022 18:40:32 GMT, XenoAmess wrote:
>> as title.
>
> XenoAmess has updated the pull request incrementally with one additional
> commit since the last revision:
>
> do it as naotoj said
Reviewed i18n-related changes and they look good. One minor suggestion in
`Calendar`, but that can be applied later.
src/java.base/share/classes/java/util/Calendar.java line 2648:
> 2646: set.add("gregory");
> 2647: set.add("buddhist");
> 2648: set.add("japanese");
This can be replaced with `SET = Set.of("gregory", "buddhist", "japanese");`.
-
Marked as reviewed by naoto (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v17]
On Fri, 27 May 2022 18:40:32 GMT, XenoAmess wrote: >> as title. > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > do it as naotoj said Reviewers for i18n, net, nio, and security, please review call site changes in your areas. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v17]
On Fri, 27 May 2022 18:40:32 GMT, XenoAmess wrote: >> as title. > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > do it as naotoj said test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 441: > 439: } > 440: } > 441: This unifies the test cases between the Set and Map factories, which accomplishes the primary goal. Good. The unification is achieved through classic object-oriented polymorphism, which works fine, though which is rather verbose. This could probably be reduced with some tinkering on the model, but it's probably reaching the point where additional tinkering on the model isn't worth it. I'm ok with sticking with this approach for now. Maybe we can clean it up later, or maybe not -- it's at least fairly straightforward. One issue that contributes to the verbosity is the repeated null checking. The null checking enables the test code to proceed and end up with -1 as the capacity if there's a null in there somewhere. This will cause the assertion to fail. This is good in that it will call attention to itself (as opposed to silently passing or something). However, if the test cases are set up properly, they should never run into null. If the null checking weren't done, an unexpected null will throw NPE, which will be caught be the framework and reported as an error. That seems perfectly fine to me, so I'd suggest simply removing the null checking. That would also reduce the bulkiness of infrastructure. - PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v17]
On Fri, 27 May 2022 18:40:32 GMT, XenoAmess wrote: >> as title. > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > do it as naotoj said 'the new' fix should be applied to newHashMap etc. too. - PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v17]
On Fri, 27 May 2022 18:40:32 GMT, XenoAmess wrote: >> as title. > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > do it as naotoj said Reviewers, could I get a review for CSR [JDK-8287419](https://bugs.openjdk.java.net/browse/JDK-8287419) please? Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v17]
> as title. XenoAmess has updated the pull request incrementally with one additional commit since the last revision: do it as naotoj said - Changes: - all: https://git.openjdk.java.net/jdk/pull/8302/files - new: https://git.openjdk.java.net/jdk/pull/8302/files/230a767e..98bfb0e1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8302=16 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8302=15-16 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8302.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8302/head:pull/8302 PR: https://git.openjdk.java.net/jdk/pull/8302
