On Mon, 11 Dec 2023 22:04:23 GMT, Justin Lu <j...@openjdk.org> wrote:
>> 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. Can you compare the built binary `currency.data` before and after your change? If they are identical, I think we can go ahead and remove the redundancy in the tool. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17023#discussion_r1423251717
