On Mon, 11 Dec 2023 19:38:38 GMT, Naoto Sato <na...@openjdk.org> wrote:
>> Justin Lu has updated the pull request with a new target base due to a merge >> or a rebase. The incremental webrev excludes the unrelated changes brought >> in by the merge/rebase. The pull request contains five additional commits >> since the last revision: >> >> - Merge branch 'master' into 8321480-ISO4217-176 >> - add comment >> - reflect review comments >> - add xcg to CurrencyNames >> - init > > make/jdk/src/classes/build/tools/generatecurrencydata/GenerateCurrencyData.java > line 347: > >> 345: // otherwise it will leak out into >> Currency:getAvailableCurrencies >> 346: boolean notFutureCurrency = >> !Arrays.asList(specialCaseNewCurrencies).contains(currencyCode); >> 347: boolean notSimpleCurrency = tableEntry == >> INVALID_COUNTRY_ENTRY > > I'd rather not bundle up those conditions into `notSimpleCurrency`, as it > obscures the other two cases. Simply OR-ing those conditions and > `notFutureCurrency` ( or `!futureCurrency` may be more readable) would be > sufficient IMO. Well, at first I had intended for the `notSimpleCurrency` to encapsulate all 3 cases. But upon further investigation, I think the current code is redundant. The third conditional, `(tableEntry & SIMPLE_CASE_COUNTRY_FINAL_CHAR_MASK) != (currencyCode.charAt(2) - 'A')` checks if the currency's first two chars match the country code (which also implies the current currency is simple). It returns false if they match. When this is false, the other first two conditions are always false. If a currencyCode is simple, the countryCode is always defined, and the currencyCode itself is not special. Checking the first two AND the third seems redundant when the first two are always false if the third is false. For example, take the currency `ADP` which will search for the table entry of the country `AD`. Since `AD=EUR`, the third conditional returns true. We now know that we do not have a simple currency, which is more than enough to make a decision whether to add the current currency to the otherCurrency List. But the existing code now checks that `AD` is a defined country and whether `EUR` is special or not (since `AD`'s tableEntry value is associated with `EUR`). I'm not sure why we would check that `EUR` is a special currency, when we are deciding to add `ADP` as an otherCurrency. It is hard to infer without any existing comments as well. With commit [85f389d](https://github.com/openjdk/jdk/pull/17023/commits/85f389dd81893f9a58f25a663cf2a18b653bced7), the same exact values are added to the otherCurrency list. However, If we would prefer not to update such old code for the sake of safety, then I would at least like to add comments that warn which conditionals may be irrelevant. Wondering what your thoughts are regarding this. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17023#discussion_r1423176706
