ICU-20036 CurrencyPluralInfo class improve handling of OOM errors

[jefgen]

Not all code paths that allocate memory check/handle OOM errors in the CurrencyPluralInfo class.
Use an internal error code (fInternalStatus) to better report errors during clone and copy construction.

[sffc]

In some methods, like operator=, you delete without checking first, but here you first check if the pointer is not nullptr before calling delete. Personally I like the first style better, but at a minimum you should probably be consistent in your style. #Resolved

[jefgen]

Thanks Shane. I was trying to mimic the style within each function (if it checked for nullptr, then I would too. If it didn't then I didn't).
However, since I'm already making a bunch of changes it would probably be better to be consistent.
Since calling delete on nullptr is safe, it seems like it would be cleaner to omit the checks.

---

In reply to: 207376490 [](ancestors = 207376490)

[sffc]

Suggestion: change this to LocalUResourceBundlePointer. #Resolved

[jefgen]

Thanks Shane, that would likely be a good idea. :)

---

In reply to: 207378377 [](ancestors = 207378377)

[sffc]

LGTM, with some additional optional suggestions.

[sffc]

It's fine, but I think it should still be safe to use the third argument of ures_getByKeyWithFallback rather than making a new LocalUResourceBundlePointer for every level in the resource bundle tree.

[jefgen]

I thought about that too, but then I was wondering who would call close on the ResourceBundle? (and in the correct order).
IIUC, calling orphan() doesn't call the close function, and if you call getAlias(), then the original bundle might be overwritten before the adoptInstead() can call close.

Ex:
rb.adoptInstead(ures_getByKeyWithFallback(numElements.getAlias(), ns->getName(), rb.orphan(), &ec));

rb.adoptInstead(ures_getByKeyWithFallback(numElements.getAlias(), ns->getName(), rb.getAlias(), &ec));

---

In reply to: 207658933 [](ancestors = 207658933)

[sffc]

What I meant was, I think it is safe to do,

LocalUResourceBundlePointer rb(ures_open(...));
ures_getByKeyWithFallback(rb.getAlias(), "...", rb.getAlias(), &ec);
ures_getByKeyWithFallback(rb.getAlias(), "...", rb.getAlias(), &ec);
ures_getByKeyWithFallback(rb.getAlias(), "...", rb.getAlias(), &ec);

Example of a place in ICU code that uses this pattern already:

icu/icu4c/source/i18n/reldtfmt.cpp

Line 450 in 778d0a6

LocalUResourceBundlePointer rb(ures_open(NULL, localeID, &status));

I'm double-checking with Markus that this is safe.

[sffc]

After studying uresbund.cpp, I can't find any code paths in ures_getByKeyWithFallback and related functions that obviously return a different object than the one you passed via the fillIn parameter. Markus didn't have an answer. The documentation also does not provide any clues.

There is one thing that we can assume, and it's that when you pass a fillIn parameter to ures_getByKeyWithFallback, that you are passing ownership to that function, because most call sites, including the one you are refactoring, overwrite the original rb pointer with the return value. In most cases, ures_getByKeyWithFallback hands ownership of fillIn right back again by returning the same pointer; the question which I can't give a 100% certain answer to is whether fillIn is always the return value when it is non-null.

What I can see by looking at the code is that you should be able to expect better performance by using the fillIn, because without the fillIn, you add extra mallocs.

I think the pattern I posted above is probably fine, since I can find other places in ICU code that use it, but if you are hesitant, then I think the following pattern is also definitely safe:

LocalUResourceBundlePointer rb(ures_open(...));
rb.adoptInstead(ures_getByKeyWithFallback(rb.getAlias(), "...", rb.orphan(), &ec));
rb.adoptInstead(ures_getByKeyWithFallback(rb.getAlias(), "...", rb.orphan(), &ec));
rb.adoptInstead(ures_getByKeyWithFallback(rb.getAlias(), "...", rb.orphan(), &ec));

rb.getAlias() gets called first, then rb.orphan(), then ures_getByKeyWithFallback does its stuff, then you save the result back into rb.

[jefgen]

Thanks Shane.
My hesitation about using that exact pattern was more about the question of "who calls the ures_close" once the code is done with the Resource Bundle, in order to ensure that there are no leaks. I was thinking that when you pass the fillIn it might not close.

However, from taking a close look at the code, it looks like it will automatically call the appropriate close methods if the fillIn argument is not NULL.
Inside the init_resb_result function:

    if(resB == NULL) {
        // .... omitted...
    } else {
        if(resB->fData != NULL) {
            entryClose(resB->fData);
        }
        if(resB->fVersion != NULL) {
            uprv_free(resB->fVersion);
        }
        // .... omitted...
    }

So, yes I would agree that it is safe in terms of not leaking when you use this pattern.

In terms of the pattern below, I think that even if getByKeyWithFallback did allocate a new object, it would still modify the incoming argument/parameter fillIn.

Pattern:

LocalUResourceBundlePointer rb(ures_open(...));
ures_getByKeyWithFallback(rb.getAlias(), "...", rb.getAlias(), &ec);

ICU Code:

U_CAPI UResourceBundle* U_EXPORT2 
ures_getByKeyWithFallback(const UResourceBundle *resB, 
                          const char* inKey, 
                          UResourceBundle *fillIn, 
                          UErrorCode *status) {
      // .....

                fillIn = init_resb_result(&(dataEntry->fData), res, inKey, -1, dataEntry, resB, 0, fillIn, status);
            } else {
                *status = U_MISSING_RESOURCE_ERROR;
            }
        } else {
            fillIn = init_resb_result(&(resB->fResData), res, key, -1, resB->fData, resB, 0, fillIn, status);
        }

So the rb object might be pointing at a new object after the call if this occurred. But that still would be safe as the close would be called when the LocalUResourceBundlePointer object goes out of scope.

[sffc]

Optional: this assignment fPluralRules = nullptr; is redundant with the following line that just reassigns it. Feel free to leave as-is if you think it increases readability. #Resolved

[jefgen]

Thanks Shane. I will push another commit that address this, but I think I will leave the other LocalUResourceBundlePointers for now. #Resolved

[sffc]

LGTM; there are a lot of commits here so I suggest a squash-merge, but you can do a merge commit if you want to preserve the history.

[jefgen]

Thank you for the detailed review Shane!
I did a squash-merge and consolidated some of the commit messages into the single commit message on the squashed commit.