Skip to content

Add Either.retract - #13731

Merged
nojb merged 3 commits into
ocaml:trunkfrom
dbuenzli:either-retract
Jan 14, 2025
Merged

Add Either.retract#13731
nojb merged 3 commits into
ocaml:trunkfrom
dbuenzli:either-retract

Conversation

@dbuenzli

Copy link
Copy Markdown
Contributor

In the discussion about Result.retract (#13721), @gasche mentioned that a similar function was equally useful on Either.t values.

I can't agree more. And the day I find it missing from the toolbox I would hate myself for not having added it now.

So here goes the trivial addition.


P.S. I find it slightly untidy that making changes to the stdlib modules makes this test fail. Isn't there perhaps another way to test what is being tested there ?

@dra27 dra27 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@dra27

dra27 commented Jan 13, 2025

Copy link
Copy Markdown
Member

P.S. I find it slightly untidy that making changes to the stdlib modules makes this test fail. Isn't there perhaps another way to test what is being tested there ?

Yeah, we could either strip the IDs off (I've debated doing this for some of the other "dump"-based tests in the past) or at least canonicalise them (via yet another "internal" flag) so

(example_1/313 =
     (function param/338[int]

always gets changed to:

(example_1/1 =
     (function param/2[int]

regardless of the actual indexes/IDs.

@nojb nojb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@nojb

nojb commented Jan 14, 2025

Copy link
Copy Markdown
Contributor

This PR can be merged once the conflict is fixed.

@gasche

gasche commented Jan 14, 2025

Copy link
Copy Markdown
Member

@dra27 I like the idea of canonicalization and I think it should be the default. (We could have a flag that disables it, so that one can compare the stamps in the dlambda output and their remnants in the generated assembly, but I think that needing this is very rare.) In particular, when there is no shadowing in play, we don't need to show the stamp at all, resulting in very readable output.

I've myself given this broad topic a bit of thought in the past. My idea was that we could tweak the pattern-matching compiler to choose suffixed names for its *match* identifiers, so that we can actually use -dno-unique-ids without being confused by the output. But this requires a non-trivial change to the pattern-matching compiler and I was never quite motivated enough to do it. Your idea of canonicalization is better, in any case.

@nojb
nojb merged commit 49baed7 into ocaml:trunk Jan 14, 2025
@dbuenzli
dbuenzli deleted the either-retract branch January 14, 2025 19:06
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.

4 participants