Skip to content
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

[Parser] Templatize lexing of integers #6272

Merged
merged 1 commit into from
Feb 5, 2024
Merged

Conversation

tlively
Copy link
Member

@tlively tlively commented Feb 2, 2024

Have a single implementation for lexing each of unsigned, signed, and
uninterpreted integers, each generic over the bit width of the integer. This
reduces duplication in the existing code and it will make it much easier to
support lexing more 8- and 16-bit integers.

Have a single implementation for lexing each of unsigned, signed, and
uninterpreted integers, each generic over the bit width of the integer. This
reduces duplication in the existing code and it will make it much easier to
support lexing more 8- and 16-bit integers.
@tlively tlively requested a review from kripken February 2, 2024 01:27
@tlively
Copy link
Member Author

tlively commented Feb 2, 2024

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@@ -100,7 +100,7 @@ inline std::optional<uint64_t> ParseInput::takeOffset() {
if (subLexer == subLexer.end()) {
return {};
}
if (auto o = subLexer->getU64()) {
if (auto o = subLexer->getU<uint64_t>()) {
Copy link
Member

Choose a reason for hiding this comment

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

getU<uint64_t> mentions "unsigned" twice. Can this be get<uint64_t>? (maybe using https://en.cppreference.com/w/cpp/types/is_unsigned)

Copy link
Member Author

Choose a reason for hiding this comment

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

getU and getI both return unsigned integers, so I think it's good to differentiate them in the method name and not favor one over the other (and all the other getXXX methods) by giving it the shorter get name.

One alternative I considered was to make the template parameter just 32 or 64, etc. but that would require more complex machinery in the implementation and the nicer API didn't seem worth the extra complexity. If you're interested, I can add a commit with that design so we can see the difference, though.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, maybe I've forgotten what those functions mean. I don't see comments on

binaryen/src/parser/lexer.h

Lines 128 to 135 in 845e070

std::optional<uint64_t> getU64() const;
std::optional<int64_t> getS64() const;
std::optional<uint64_t> getI64() const;
std::optional<uint32_t> getU32() const;
std::optional<int32_t> getS32() const;
std::optional<uint32_t> getI32() const;
std::optional<double> getF64() const;
std::optional<float> getF32() const;

Perhaps it's worth adding some? Reading the code, the meaning seems to be "U is unsigned, S is signed, I accepts both as inputs but returns unsigned" - ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's right. uN, sN, and iN come directly from the grammar in the spec, but I can add comments here as well.

Copy link
Member

Choose a reason for hiding this comment

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

Right, sorry, I keep reading this code as normal C++ and I forget it mirrors the spec...

@tlively tlively merged commit ed15efe into main Feb 5, 2024
15 checks passed
@tlively tlively deleted the parser-template-ints branch February 5, 2024 18:24
radekdoulik pushed a commit to dotnet/binaryen that referenced this pull request Jul 12, 2024
Have a single implementation for lexing each of unsigned, signed, and
uninterpreted integers, each generic over the bit width of the integer. This
reduces duplication in the existing code and it will make it much easier to
support lexing more 8- and 16-bit integers.
@gkdn gkdn mentioned this pull request Aug 31, 2024
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.

2 participants