Skip to content

MetaOCaml: the parser.mly change only#13257

Merged
gasche merged 4 commits into
ocaml:trunkfrom
gasche:MetaOCaml_parsing_only
Jul 2, 2024
Merged

MetaOCaml: the parser.mly change only#13257
gasche merged 4 commits into
ocaml:trunkfrom
gasche:MetaOCaml_parsing_only

Conversation

@gasche
Copy link
Copy Markdown
Member

@gasche gasche commented Jun 19, 2024

This is a simplified version of #12470 that only includes the parser.mly changes -- no lexing support, and therefore no backward-compatibility questions of any kind (great!) and no ability to test the new code either (meh...).

(what follows is reused from the #12470 description to provide more context)


Context

@Octachron and myself are working with Oleg Kiselyov and @yallop to restart upstreaming parts of the BER MetaOCaml codebase -- thanks to a helpful prompt from @haochenx at ICFP 2022.

The current goal is not to make MetaOCaml features available to all OCaml users, but to reduce the diff between upstream OCaml and the MetaOCaml fork to make it easier for MetaOCaml maintainers (Oleg and @yallop) to port the MetaOCaml patches to newer OCaml versions. We can upstream things gradually in this way, guided by the MetaOCaml maintainers' experience of what parts of the codebase generate more maintenance work than others.

(This approach was suggested by Oleg, who is not in favor of upstreaming all of MetaOCaml, as this is a research project that still evolves in backwards-incompatible way on a regular basis.)

This PR

This PR upstreams the MetaOCaml logic for parsing (but not lexing) MetaOCaml constructs. This was identified as a high-maintenance-cost area by the MetaOCaml maintainers.

@gasche gasche mentioned this pull request Jun 19, 2024
Comment thread parsing/parser.mly Outdated
Comment thread boot/menhir/parser.mli Outdated
| HASH
| GREATERRBRACKET
| GREATERRBRACE
| GREATERDOT
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.

Don't we want to abstract the token name since we disagree on the concrete token?
Maybe

  • META_BRACKET_CLOSE
  • META_BRACKET_OPEN
  • META_ESCAPE
    ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I thought about this, but then I decided that if the goal is to make Oleg's life easier, maybe forcing non-essential renames was not the best approach?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

(I'm happy to do the rename if you thought about this even slightly more and still believe that it's the right approach.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This aligns with a suggestion from Oleg (in an email thread), so I did the rename. I used METAOCAML_ instead of META_ as a prefix.

Co-authored-by: Gabriel Scherer <gabriel.scherer@gmail.com>
Reviewed-by: Florian Angeletti <florian.angeletti@inria.fr>
@gasche gasche force-pushed the MetaOCaml_parsing_only branch from 7d9a8c1 to 6b7ea6c Compare June 23, 2024 13:12
Copy link
Copy Markdown
Member

@yallop yallop left a comment

Choose a reason for hiding this comment

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

This looks good to me. (I assume the testsuite changes are also just to ease the MetaOCaml maintenance burden; they don't seem strictly necessary.)

@gasche
Copy link
Copy Markdown
Member Author

gasche commented Jul 1, 2024

Indeed, the testsuite changes are relics of the previous patch which introduced surface-language support (and which conflicted with a couple occurrences of .< in the testsuite). They are harmless and it may also simplify things down the line, so I kept them even though they are not necessary anymore.

I pushed a Changes entry and will merge when the CI agrees.

@gasche gasche added the merge-me label Jul 1, 2024
@gasche gasche merged commit 6bc1966 into ocaml:trunk Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants