Skip to content

Conversation

@catamorphism
Copy link
Contributor

@catamorphism catamorphism commented Jun 28, 2025

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
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

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

@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

@catamorphism catamorphism force-pushed the bidi-default-strategy branch from 267e8e3 to df7d54a Compare January 14, 2026 00:58
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

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

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@catamorphism
Copy link
Contributor Author

I'm not sure why the format checker is failing, as this PR doesn't change any Java files.

@mihnita mihnita requested a review from roubert January 16, 2026 00:06
@roubert
Copy link
Member

roubert commented Jan 16, 2026

I'm not sure why the format checker is failing, as this PR doesn't change any Java files.

While it won't solve the mystery of why ICU4J / Format checker was being run at all for this PR, you might be able to make the error go away by rebasing on top of current main, commit 148f5ca.

@mihnita
Copy link
Contributor

mihnita commented Jan 16, 2026

  1. For java formatting, my PR landed, you would have to merge that into your branch

  2. For single commit, I recommend you squash

Checking the 9 commits, there is nothing that is worth keeping separate for easier review, or better history. They are just attempts to fix problems. Happen, they are natural process, but not worth preserving.
If you don't have the right to squash from the GitHub UI, let me know and I'l do it.

Squashing will also solve the "jira ticket", which complains about the description (the squashed commit will have a description that passes the test).

And fixing all 3 complaints will also fix the general one "waiting tor required checks" and you will have a green PR.

@catamorphism catamorphism force-pushed the bidi-default-strategy branch from b244089 to 7abcb4b Compare January 19, 2026 20:39
@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
Copy link
Contributor Author

I've rebased and squashed; hoping this fixes all the CI errors 🤞🏼

…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 catamorphism force-pushed the bidi-default-strategy branch from 7abcb4b to ccfe7bd Compare January 20, 2026 23:50
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/i18n/messageformat2_function_registry_internal.h is different
  • icu4c/source/i18n/messageformat2.cpp is different
  • icu4c/source/i18n/unicode/messageformat2_data_model.h is different
  • icu4c/source/i18n/unicode/messageformat2_formattable.h is different
  • icu4c/source/i18n/unicode/messageformat2_function_registry.h is different
  • icu4c/source/i18n/unicode/messageformat2.h is different
  • testdata/message2/spec/u-options.json is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@catamorphism
Copy link
Contributor Author

catamorphism commented Jan 20, 2026

I've removed support for the u:locale option and force-pushed. To the best of my knowledge, this PR now reflects the draft LDML 49 spec except:

  • :currency, :percent, and :offset are not supported.
  • date/time formatting is based on an earlier version of the spec; since date/time formatting still has Draft status in the spec, I'm waiting for that to be finalized before making more changes to date/time formatting.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants