Skip to content

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.

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
@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

2 participants