ICU-20558 Fix resource fallback logic in ures_getFunctionalEquivalent #621
Conversation
| @@ -2632,7 +2632,8 @@ ures_getFunctionalEquivalent(char *result, int32_t resultCapacity, | |||
| #endif | |||
| if(U_FAILURE(subStatus)) { | |||
| *status = subStatus; | |||
| } else if(subStatus == U_ZERO_ERROR) { | |||
| } else if(subStatus == U_ZERO_ERROR || subStatus == U_USING_DEFAULT_WARNING) { | |||
There was a problem hiding this comment.
I still reproduced the issue in the latest master. How does commit #092afb3 fix this issue?
There was a problem hiding this comment.
Particularly, it's a regression in ICU 63 or lower. Do you know which commmit caused the regression?
There was a problem hiding this comment.
How does commit #092afb3 fix this issue?
It does not, it fixes a different (but somewhat related) issue.
Do you know which commmit caused the regression?
Sorry, no.
There was a problem hiding this comment.
Actually, since your change was not accepted, I guess I suspect that an invalid locale "abc" should not fallback to the default locale.
But I think it should at least fallback to the root locale. What do you think?
There was a problem hiding this comment.
b12a927
this commit caused the regression.
Specifically, the new error check in DateTimePatternGenerator::getCalendarTypeToUse
There was a problem hiding this comment.
Thanks for tracking down the change @gvictor.
Here is a link to the line in the PR diff:
b12a927#diff-14f1d714f00a6f21e52eae73ad7fa1e4R771
The change added a check for failure after calling ures_getFunctionalEquivalent, as before there was no check at all.
If ures_getFunctionalEquivalent failed (imagine an out-of-memory failure) then the code would continue and would eventually return U_ZERO_ERROR.
I don't think hiding catastrophic errors like OOM is good practice, so I'm somewhat inclined to keep the check in place. Perhaps it could be weakened to allow U_MISSING_RESOURCE_ERROR though?
| @@ -2632,7 +2632,8 @@ ures_getFunctionalEquivalent(char *result, int32_t resultCapacity, | |||
| #endif | |||
| if(U_FAILURE(subStatus)) { | |||
| *status = subStatus; | |||
| } else if(subStatus == U_ZERO_ERROR) { | |||
| } else if(subStatus == U_ZERO_ERROR || subStatus == U_USING_DEFAULT_WARNING) { | |||
| subStatus = U_ZERO_ERROR; | |||
| ures_getByKey(res,resName,&bund1, &subStatus); | |||
| if(subStatus == U_ZERO_ERROR) { | |||
There was a problem hiding this comment.
^ subStatus was just compared 2 lines before
|
Thanks @jefgen . I am closing this PR in favour of your change. |
https://unicode-org.atlassian.net/browse/ICU-20558