ICU-22261 Refactor MF2 attributes and options parsing code

[catamorphism]

Previously, there were separate overrides for the options and attributes parsing methods in the parser that were used in different context. (Options can appear in Operator and Markup, while attributes can appear in Expression and Markup.)

This is a refactoring that eliminates this duplicated code. To enable it, a builder is added for the internal OptionMap type.

This doesn't change any public APIs, but is also not an urgent change.

Checklist

• Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22261
• Required: The PR title must be prefixed with a JIRA Issue number.
• Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
• Required: Each commit message must be prefixed with a JIRA Issue number.
• Issue accepted (done by Technical Committee after discussion)
• Tests included, if applicable
• API docs and/or User Guide docs changed or added, if applicable

[catamorphism]

/cc @markusicu @echeran @mihnita

[markusicu]

Assuming this is still for ICU 75, it needs to now target the https://github.com/unicode-org/icu/tree/maint/maint-75 branch rather than "main".

[echeran]

LGTM. Thanks!

[catamorphism]

The CI failures are jobs that timed out. Not sure if there's a way to re-run them.

[echeran]

I clicked rerun on the failed CI checks on this PR yesterday when I noticed it. If you click the "details" link, there (hopefully) is a dropdown button in the upper right that lets you rerun jobs (all jobs & just failing jobs). Once you click the button to get the failing jobs to rerun, the button won't be visible until there's another occurrence of a failed (finished) job. If you don't see or have the ability to click the rerun jobs button, just ping me and I'll be happy to poke this thing until it passes.

[catamorphism]

After discussing with Elango, I added 2497b11 to increase the timeouts for the two MSVC debug jobs that are timing out.

[catamorphism]

Hmm, I'm not sure why the MSVC debug jobs were being reported as timeouts, because when I looked at the logs just now, there are actually test failures. I'll look at that tomorrow.

[echeran]

Based on the log output, it looks like the failed runtime assertion caused the system to give an interactive prompt:

Assertion failed: !other.bogus, file D:\a\1\s\icu4c\source\i18n\messageformat2_data_model.cpp, line 275
##[error]The Operation will be canceled. The next steps may not contain expected logs.
Terminate batch job (Y/N)? 

At least we can revert 2497b11 that has the increases in the timeout length for the respective jobs.

[catamorphism]

The last few commits should fix the build failures on MSVC.

If anyone is curious:

1. An assertion was wrong, which I fixed in 78f334c. This should have caused an assertion failure in any debug build, but it didn't, only on MSVC. This is troubling, but I don't have an explanation.

2. Once I fixed that, it exposed a double-free bug when calling ~OptionMap::Builder(). I realized this was because the Builder classes all have implicit copy constructors and copy assignment operators, since I didn't explicitly delete them. This wasn't causing problems before because of how the Builder constructors were called. However, OptionMap has a factory method (introduced in this PR) with code like:

OptionMap::Builder b;
// ...
return b;

I suspect that the other compilers were doing copy elision here, and MSVC wasn't. It would be a bug to call the implicit copy constructor for OptionMap::Builder, since it has a pointer as a member. I fixed this by explicitly deleting the constructors and adding an explicit move constructor and by-value assignment operator. While I was at it, I did the same for the other Builder classes. (The public ones are not supposed to be copyable or movable. OptionMap::Builder is non-public, so it can work differently.)

[echeran]

LGTM. All checks are passing now. Good work in debugging and diagnosing.

[echeran]

@markusicu @roubert FYI on Tim's comment above, where a failing debug assert theoretically should have caused failures on all compiler builds across the board, but only seemed to cause a failure with MSVC. Is there a way to explain this?

[roubert]

@markusicu @roubert FYI on Tim's comment above, where a failing debug assert theoretically should have caused failures on all compiler builds across the board, but only seemed to cause a failure with MSVC. Is there a way to explain this?

I can't find any unit test in which copyVectorToArray() returns nullptr in OptionMap::OptionMap(). If that's so, then that would explain why no test failed before the bugfix. If that isn't so, which test case exactly is it that exercies this code path?

[catamorphism]

@markusicu @roubert FYI on Tim's comment above, where a failing debug assert theoretically should have caused failures on all compiler builds across the board, but only seemed to cause a failure with MSVC. Is there a way to explain this?

I can't find any unit test in which copyVectorToArray() returns nullptr in OptionMap::OptionMap(). If that's so, then that would explain why no test failed before the bugfix. If that isn't so, which test case exactly is it that exercies this code path?

The test that fails is https://github.com/unicode-org/icu/blob/main/icu4c/source/test/intltest/messageformat2test.cpp#L411 . There are no unit tests specifically for OptionMap (yes, test coverage needs some work).

I can do some more work to narrow this down before you put any more time into it.

[catamorphism]

I've convinced myself that I understand why the assertion only failed on Windows; it was for the same reason as the second bug (unintentionally using a default copy constructor). copyVectorToArray() was passed an uninitialized vector (because of the bad copy constructor) so it returned nullptr and set len to 0; the assertion that was failing in its caller checked for nullptr.

I've also convinced myself that the correct fix is actually not to change the assertion, but rather, to change copyVectorToArray() and similar methods to take an error code, rather than setting a reference parameter to 0 to indicate failure. I'm going to do that in a separate PR.

Meanwhile, I'll squash this so it can be merged.

[jira-pull-request-webhook]

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

~ Your Friendly Jira-GitHub PR Checker Bot

[catamorphism]

I submitted #2954 to change the error handling to be more in line with ICU style -- that will have to be rebased once this PR lands, or alternately, this PR could be rebased off #2954. I won't have a chance to do either until April 16 (will be on vacation).