Skip to content

Conversation

@tlively
Copy link
Member

@tlively tlively commented May 26, 2022

wat-parser-internal.h was already quite large after implementing just the lexer,
so it made sense to rename it to be lexer-specific and start a new file for the
higher-level parser. Also make it a proper .cpp file and split the testable
interface out into wat-lexer.h.

@tlively
Copy link
Member Author

tlively commented May 26, 2022

@tlively tlively force-pushed the lex-str-escaped-rename branch from 1875a2b to acbd1f4 Compare May 26, 2022 23:25
Base automatically changed from lex-str-escaped-rename to main May 27, 2022 01:00
tlively added 2 commits May 26, 2022 18:57
wat-parser-internal.h was already quite large after implementing just the lexer,
so it made sense to rename it to be lexer-specific and start a new file for the
higher-level parser. Also make it a proper .cpp file and split the testable
interface out into wat-lexer.h.
Copy link
Member

@aheejin aheejin left a comment

Choose a reason for hiding this comment

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

You said you named the previous header wat-parser-internal.h because this was not meant to be used from outside, and you were gonna create a new header with public API. Now these headers don't have internal in the names, are they meant to be public? But this has the same contents as the previous wat-parser-internal.h.

@@ -0,0 +1,1004 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

Huh, I wonder why git doesn't recognize this as a file move wat-parser.cpp->wat-lexer.cpp..

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like it does now 👍

@tlively
Copy link
Member Author

tlively commented May 27, 2022

If there were a good way to make a hierarchy of headers, then I would make wat-lexer.h a private header within the parser module so that it could be used from a future wat-parser.cpp and from nowhere else. Unfortunately there isn't a great way to do that in C++. If we're going to split the lexer's interface declarations from its implementation (which I think makes sense given how large the implementation is), then I don't see any benefit in marking the header "internal" anymore.

I doubt any other code in binaryen will want to use the lexer directly, but if it does, that would be ok after this change.

@tlively tlively requested review from aheejin and kripken May 27, 2022 17:34
@tlively
Copy link
Member Author

tlively commented May 27, 2022

Graphite Merge Job

Current status: ✅ Merged

This pull request was successfully merged as part of a stack.

This comment was auto-generated by Graphite.

Job Reference: UksnMFlJsREOdJP1qpqD

@tlively tlively merged commit 404bd2c into main May 27, 2022
@tlively tlively deleted the lexer-header branch May 27, 2022 20:28
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