[NFC] Refactor code to reduce boilerplate for new code annotations #8246
Conversation
| return withLoc(pos, | ||
| irBuilder.makeIf(label ? *label : Name{}, | ||
| type.getSignature(), | ||
| parseAnnotations(annotations))); |
There was a problem hiding this comment.
Should we just pass a CodeAnnotation instead of a const std::vector<Annotation>& into all of these makeXYZ functions?
There was a problem hiding this comment.
Possibly, good question... Looking around a little, that would change hundreds of lines in the parser, so seems like a large refactoring. Perhaps we can consider it after?
src/wasm-ir-builder.h
Outdated
| makeIf(Name label, Signature sig, std::optional<bool> likely = std::nullopt); | ||
| Result<> makeIf(Name label, | ||
| Signature sig, | ||
| const CodeAnnotation& annotations = CodeAnnotation()); |
There was a problem hiding this comment.
It should be possible to slightly shorten this and reduce duplication:
| const CodeAnnotation& annotations = CodeAnnotation()); | |
| const CodeAnnotation& annotations = {}); |
| @@ -1297,39 +1297,63 @@ struct ParseImplicitTypeDefsCtx : TypeParserCtx<ParseImplicitTypeDefsCtx> { | |||
| }; | |||
|
|
|||
| struct AnnotationParserCtx { | |||
There was a problem hiding this comment.
Not really related to this change, but it looks like parseAnnotations could just be a free function instead of accessed via multiple inheritance from AnnotationParserCtx.
There was a problem hiding this comment.
Seems reasonable either way to me, I don't feel strongly.
src/wasm/wasm-binary.cpp
Outdated
| branchHintsLen = payloadLen; | ||
| // Deferred. | ||
| deferredAnnotationSections.push_back( | ||
| AnnotationSectionInfo{pos, [=, this]() { this->readBranchHints(payloadLen); }}); |
There was a problem hiding this comment.
Listing this shouldn't be necessary, and apparently isn't even valid before C++20? (link)
| AnnotationSectionInfo{pos, [=, this]() { this->readBranchHints(payloadLen); }}); | |
| AnnotationSectionInfo{pos, [=]() { this->readBranchHints(payloadLen); }}); |
There was a problem hiding this comment.
Yes, apparently combining = with this is bad... and various fixes hit various warnings, which I found tricky to get passing in both our normal and c++20 testing. The final code seems to work, using
[this, payloadLen]() { this->readBranchHints(payloadLen); }
|
Do I understand that this has the (probably inconsequential) side effect of preserving inline hints on |
|
Yes, this generalization does mean we handle all hints in more places. As you say it seems inconsequential, and it would add complexity to avoid. |
2 participants
CodeAnnotationout ofFunction, for easier use.parseAnnotations), returninga
CodeAnnotation. This avoids separate parse this/parse thatscattered around: all such places now just pass around a
CodeAnnotation, and they are all applied in one place,applyAnnotations.XPos, XLenpairs in the binary reader: just use a vector oflambdas to call, for deferred annotation parsing.