Skip to content

Modality.Value.Const should behave like a product #3961

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.

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

Merged
merged 6 commits into from
May 22, 2025

Conversation

riaqn
Copy link
Contributor

@riaqn riaqn commented May 2, 2025

Based on #3949

This PR refactors the interface of Modality.Value.Const to make it behave like a product of axes (that you can project and update specific axis) instead of a sequence of atoms (that you compose).

This allows other incoming PRs that do the following:

  • Remove @no_mutable_implied_modalities #3962
  • fix the behavior of default modalities, so the overriding is per-axis.
  • (done in this PR) warnings for specifying the same modality axis multiple times in a single modality expression.
  • warnings for superfluous overriding (e.g., overriding portable with portable)

Review

@dkalinichenko-js , could you look at the changes in typemode.ml and tests. In particular, could you check that the implication and the print back is still as expected. Feel free to add more tests.

@riaqn riaqn requested a review from dkalinichenko-js May 2, 2025 13:47
@riaqn riaqn marked this pull request as draft May 2, 2025 14:06
@riaqn riaqn force-pushed the refactor-modality-const-repr branch from 2f81160 to f37af34 Compare May 2, 2025 17:29
@dkalinichenko-js dkalinichenko-js force-pushed the refactor-modality-const-repr branch from f37af34 to f852f50 Compare May 20, 2025 18:31
@dkalinichenko-js dkalinichenko-js force-pushed the clean-up-mode-axis-gadt branch from 9f251f0 to 7260beb Compare May 20, 2025 18:36
@dkalinichenko-js dkalinichenko-js force-pushed the refactor-modality-const-repr branch from f852f50 to aa8de35 Compare May 20, 2025 18:39
@riaqn riaqn marked this pull request as ready for review May 21, 2025 01:56
@riaqn
Copy link
Contributor Author

riaqn commented May 21, 2025

Thank you @d-kalinichenko ! I reviewed your fixes and they look great!
I did some further clean up, and it should be ready for review now.

Copy link
Contributor

@dkalinichenko-js dkalinichenko-js left a comment

Choose a reason for hiding this comment

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

LGTM modulo the order comments.

Base automatically changed from clean-up-mode-axis-gadt to main May 22, 2025 03:01
@riaqn riaqn force-pushed the refactor-modality-const-repr branch from 57a5d4b to bf120a4 Compare May 22, 2025 03:03
@riaqn riaqn merged commit f15cd33 into main May 22, 2025
28 checks passed
@riaqn riaqn deleted the refactor-modality-const-repr branch May 22, 2025 05:19
Gbury pushed a commit to Gbury/flambda-backend that referenced this pull request May 27, 2025
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.

3 participants