ICU-22779 Fixed ICU4C DataBuilderCollationIterator initialization order issue

[yumaoka]

There is an initialization ordering problem in the DataBuilderCollationIterator constructor. GCC 14 with -Werror=uninitialized will cause an compilation error.

Checklist

• Required: Issue filed: ICU-22779
• Required: The PR title must be prefixed with a JIRA Issue number. Example: "ICU-NNNNN Fix xyz"
• Required: Each commit message must be prefixed with a JIRA Issue number. Example: "ICU-NNNNN Fix xyz"
• Issue accepted (done by Technical Committee after discussion)
• Tests included, if applicable
• API docs and/or User Guide docs changed or added, if applicable
• Approver: Feel free to merge on my behalf

[yumaoka]

@markusicu ICU4J implementation takes CollationData in the constructo as below -

    private static final class DataBuilderCollationIterator extends CollationIterator {
        DataBuilderCollationIterator(CollationDataBuilder b, CollationData newData) {
            super(newData, /* numeric= */ false);

In this PR, I kept the constructor's signature unchanged, but added CollationIterator constructor which allows a subclass to set CollationData later. Do you want to make a change the DataBuilderCollationIterator to have a pointer to CollationData, then delete it in the destructor? Or, are you OK with this change (CollationIterator to initialize members in the new protected constructor and a separated setter method)?

[markusicu]

@markusicu ICU4J implementation takes CollationData in the constructo as below -

    private static final class DataBuilderCollationIterator extends CollationIterator {
        DataBuilderCollationIterator(CollationDataBuilder b, CollationData newData) {
            super(newData, /* numeric= */ false);

In this PR, I kept the constructor's signature unchanged, but added CollationIterator constructor which allows a subclass to set CollationData later. Do you want to make a change the DataBuilderCollationIterator to have a pointer to CollationData, then delete it in the destructor? Or, are you OK with this change (CollationIterator to initialize members in the new protected constructor and a separated setter method)?

Interesting... We construct the base class with a pointer to the buildData field (type: CollationData) which isn't initialized yet. The base class (CollationIterator) stores the pointer itself, which should be fine; and it also copies the buildData.trie pointer, which isn't initialized yet.

Why does this work at all?
The CollationData constructor is nearly trivial; it initializes the trie pointer to nullptr.
Some later code must then create the trie and set the pointer in both the buildData and in the CollationIterator base.
So whether the constructors copy a nullptr or an uninitialized value has no effect, except making a smart compiler unhappy.

The simplest right answer is probably to add a new CollationIterator constructor that simply sets the trie pointer to nullptr.

[markusicu]

PS: It looks like CollationIterator has both a CollationData pointer and a trie pointer which is CollationData->trie “only as an optimization”. Which means that, if we didn't care about an additional pointer indirection, we might be able to simplify this by removing the CollationIterator.trie field and always using an indirect access via data->trie.

I don't know if common compilers can be relied on optimizing this to the equivalent code.

[jira-pull-request-webhook]

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

[markusicu]

Sorry, follow-up questions.

---

Re #4111 (comment)
let's not do that -- let's keep the optimization explicit, rather than trusting the compiler.

[markusicu]

it would be nice to fix the typo; i can quickly re-stamp

[jira-pull-request-webhook]

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

[markusicu]

typo still there
rerunning failed msvc dist workflow

[jira-pull-request-webhook]

Notice: the branch changed across the force-push!

• icu4c/source/i18n/collationiterator.h is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

[markusicu]

@echeran @mihnita @yumaoka @eggrobin could i ask for a rubber stamp?
i approved yoshito's PR but then fixed a typo