On Sun, 25 Jun 2023 22:32:34 GMT, Pavel Rappo <pra...@openjdk.org> wrote:
>> I'll re-run our CI, and if all good, I'll sponsor this PR. > >> I'll re-run our CI, and if all good, I'll sponsor this PR. > > The CI tests I started have just passed. While this PR is already good, I > wonder if we make it even better. > > I doubt highly that we need null-checks for CacheKey's name and locale. My > suspicion comes from these two facts: > > * CacheKey's hashCode calls methods on these fields without checking them for > null, and > * CacheKey instances are queried and stored in ConcurrentHashMap, which > certainly uses both `equals` and `hashCode` > > So, what I'm saying is that if those fields were null, there would be > unhandled NPE somewhere; but I cannot see such NPE. Additionally, those > fields are used in toString(). Furthermore, locale is known to not be null > because the only place from where it is passed to a CacheKey consturctor is > ResourceBundle.getBundleImpl(Module, Module, String, Locale, > ResourceBundle.Control), which explicitly checks that locale for not being > null. > > Does it make sense? > @pavelrappo you mean we can revert changes to ResourceBundle.java in lines > 729 and 733? Yes, those lines. We can revert to x.equals(otherEntry.x). ------------- PR Comment: https://git.openjdk.org/jdk/pull/12328#issuecomment-1607062302
