<i18n dev> RFR: 8321480: ISO 4217 Amendment 176 Update [v2]

[Naoto Sato]

On Mon, 11 Dec 2023 22:04:23 GMT, Justin Lu <jlu at 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