On Mon, 11 Dec 2023 23:59:24 GMT, Naoto Sato <na...@openjdk.org> wrote:
>> 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. Yes, I will go ahead and compare, as well as do a few more sanity checks ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17023#discussion_r1423253808
