Skip to content

ICU-22939 Function composition and default bidi strategy #3536

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

ICU-22939 Function composition and default bidi strategy #3536

wants to merge 7 commits into from

Conversation

catamorphism
Copy link
Contributor

@catamorphism catamorphism commented Jun 28, 2025 •
edited
Loading

This PR does several things:

  1. Implements the changes to resolved values necessary to implement function composition, as described in an approved design doc.
  2. Implements lazy/call-by-need evaluation. The previous implementation used lazy/call-by-name evaluation. Call-by-need is necessary because custom functions can potentially depend on external state and thus may return different values on different calls (for example, a function that returns the system time).
  3. Implements the default bidi strategy as described in another approved design doc. The changes are related to the changes to resolved values, which is why these changes appear in a single PR.
  4. Updates spec tests to the current version of the message-format-wg repo, with the exception of currency and math tests (these functions are not yet implemented).

Note 1: There are a few differences between the code and two design docs, which I detailed in an email to the icu-design list. This PR is contingent on those changes being approved, but since there's a lot here to review, I wanted to go ahead and make the PR available for review. As there have been no objections to the email, I'm considering the additional changes approved.

Note 2: I will be on leave from July 2-July 14 and won't see review comments during that time. I'll reply to comments when I return.

Checklist

  • Required: Issue filed: ICU-22939
  • Required: The PR title must be prefixed with a JIRA Issue number. Example: "ICU-1234 Fix xyz"
  • Required: Each commit message must be prefixed with a JIRA Issue number. Example: "ICU-1234 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

@catamorphism catamorphism force-pushed the bidi-default-strategy branch from 7373296 to 24c64c9 Compare July 1, 2025 01:12
@jira-pull-request-webhook
Copy link

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

~ Your Friendly Jira-GitHub PR Checker Bot

@catamorphism catamorphism force-pushed the bidi-default-strategy branch from 24c64c9 to b34d0a5 Compare July 1, 2025 01:34
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/i18n/messageformat2_function_registry.cpp is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@catamorphism catamorphism changed the title ICU-22939 DRAFT: Function composition and default bidi strategy ICU-22939 Function composition and default bidi strategy Jul 1, 2025
@catamorphism catamorphism marked this pull request as ready for review July 1, 2025 20:29
srl295
srl295 reviewed Jul 22, 2025
View reviewed changes
Copy link
Member

@srl295 srl295 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a couple of starting comments

icu4c/source/i18n/messageformat2_allocation.h
Comment on lines +136 to +147
template<typename T>
inline T* create(const T& node, UErrorCode& status) {
if (U_FAILURE(status)) {
return nullptr;
}
T* result = new T(node);
if (result == nullptr) {
status = U_MEMORY_ALLOCATION_ERROR;
}
return result;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if these should go into common - perhaps cmemory.h , errorcode.h or uobject.h

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be done in a future PR? Would be happy to open a JIRA ticket for it. This PR is already doing a lot. cmemory.h seems like the best place, or possibly a new private header, since cmemory.h is already pretty big.

@catamorphism
Copy link
Contributor Author

I'm guessing the Windows compilation errors might have something to do with the changes in #3521. I'll look into it.

@catamorphism catamorphism force-pushed the bidi-default-strategy branch from d973ccb to 87fbe3a Compare July 23, 2025 20:37
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/i18n/unicode/messageformat2_function_registry.h is different
  • icu4c/source/i18n/unicode/messageformat2.h is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

catamorphism and others added 7 commits September 9, 2025 12:18
@catamorphism
ICU-22939 MF2: Re-implement resolved values and implement bidi defaul…
1964010 
…t strategy

Implement the changes to resolved values necessary to implement function
composition.

Implement lazy/call-by-need evaluation (instead of lazy-call-by-name
evaluation).

Implement the default bidi strategy and APIs for controlling it.

Update spec tests to those from the current version of the
message-format-wg repo, except for currency and math tests (these
functions are not yet implemented).
@catamorphism @srl295
Update icu4c/source/i18n/unicode/messageformat2.h
74da42d 
Co-authored-by: Steven R. Loomis <[email protected]>
@catamorphism @srl295
Update icu4c/source/i18n/unicode/messageformat2.h
ee12077 
Co-authored-by: Steven R. Loomis <[email protected]>
@catamorphism
Use DEFAULT values for enums
31ba84f
@catamorphism
Add U_I18N_CLASS / U_I18N macros to FunctionValue and FunctionContext
6b5100e
@catamorphism
Remove un-needed attributes
058b404
@catamorphism
Add missing attributes
267e8e3
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/i18n/unicode/messageformat2_data_model.h is different
  • icu4c/source/i18n/unicode/messageformat2_formattable.h is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@srl295 srl295 srl295 left review comments

@echeran echeran Awaiting requested review from echeran

@mihnita mihnita Awaiting requested review from mihnita

@markusicu markusicu Awaiting requested review from markusicu

+1 more reviewer

@coltbergheger1997 coltbergheger1997 coltbergheger1997 approved these changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

@mihnita mihnita

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@catamorphism @srl295 @coltbergheger1997 @mihnita