Use classes + virtual functions for BinaryReader#376
Conversation
|
Would it make sense to make |
|
@pipcet I had it that way at first, but it makes the forwarding in |
|
@binji I see. I must say I'd still prefer not to have that state argument, which appears to be unused 90% of the time. It's like having two I must confess I can't think of a perfectly clean way of doing this, my best attempt is: When parsing starts, the BinaryReader's BeginModule method is called with a That would be the least code, I think (LoggingReader can pass through the state pointer rather than having to copy data) and shouldn't reduce performance, while still allowing the state class to grow to include more state. It's a bit C-like in using actual pointers though, so I fully understand if you prefer leaving things as in the PR; either way, I think, this PR is a huge improvement. |
|
Yeah, that's not bad. I'll try it out and see. |
|
Thanks for the suggestion, @pipcet. This is much nicer. |
sbc100
left a comment
sbc100
left a comment
There was a problem hiding this comment.
Mostly looks awesome! Great to see so many casts removed
| context->section_starts[static_cast<size_t>(section_code)] = ctx->offset; | ||
| Result BinaryReaderObjdumpPrepass::OnFunctionName(uint32_t index, | ||
| StringSlice name) { | ||
| if (options->mode == ObjdumpMode::Prepass) { |
There was a problem hiding this comment.
Why is this condition needed? Shouldn't it just be split into two different methods? On a related note is seems strange for BinaryReaderObjdump to inherit from BinaryReaderObjdumpPrepass. Could they both inherit from some common base class instead?
src/binary-reader-objdump.cc
Outdated
| PrintDetails(" - %-18s idx=%#-4x addend=%#-4x offset=%#x(file=%#x)\n", | ||
| get_reloc_type_name(type), index, addend, offset, total_offset); | ||
| if (options->mode == ObjdumpMode::Prepass && | ||
| reloc_section == BinarySection::Code) { |
| /* MEMORY section data */ | ||
| Limits memory_limits; | ||
| }; | ||
| } data; |
There was a problem hiding this comment.
Kinda. I started getting a bug because this union wasn't initialized to zero. It seemed easiest to name it so I could do WABT_ZERO_MEMORY(data)
| OnReturnExpr | ||
| EndFunctionBody(0) | ||
| EndCodeSection | ||
| EndModule |
There was a problem hiding this comment.
Looks like the looking used to happen twice here? Was that am existing bug?
There was a problem hiding this comment.
Oh weird, I didn't even notice. Yeah, must have been an existing bug.
There was a problem hiding this comment.
Yeah, I forgot. the BinaryReaderInterpreter did two passes over the binary before since you need to first validate all the modifications to the data and elem segments, then only apply them if all are valid. This turned out to be a pain for the new system, so I ended up using two vectors to store the modifications and updating them later.
This adds a few new classes: * BinaryReader: the abstract base class * BinaryReaderNop: implements all of BinaryReader, but does nothing * BinaryReaderLogging: logs calls through BinaryReader, and forwards to another BinaryReader Typically this means we can remove the Context structs from these implementations, since that data can just move into the BinaryReader subclasses. I also took the opportunity to rename the new member functions to MixedCase instead of snake_case, since that's more common in C++.
3 participants
This adds a few new classes:
another BinaryReader
Typically this means we can remove the Context structs from these
implementations, since that data can just move into the BinaryReader
subclasses.
I also took the opportunity to rename the new member functions to
MixedCase instead of snake_case, since that's more common in C++.