[Remarks] BitstreamRemarkParser: Refactor error handling #156511
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Created using spr 1.3.7-wip [skip ci]
Created using spr 1.3.7-wip
fhahn
reviewed
Sep 3, 2025
| Error BitstreamRemarkParser::parseMeta() { | ||
| // Advance and to the meta block. | ||
| if (Error E = advanceToMetaBlock(ParserHelper)) | ||
| if (Error E = ParserHelper.advanceToMetaBlock()) |
There was a problem hiding this comment.
Suggested change
| if (Error E = ParserHelper.advanceToMetaBlock()) | |
| // Advance and to the meta block. | |
| if (Error E = ParserHelper.advanceToMetaBlock()) |
can keep comment?
There was a problem hiding this comment.
There's a typo, the comment doesn't really say more than the function name and advanceToMetaBlock will be removed by the follow-up patch anyways.
|
Is the main goal to unify the API for the standalone/separate parsers under |
Created using spr 1.3.7-wip [skip ci]
Created using spr 1.3.7-wip
Unfortunately, there is no good way to create malformed bitstream files for testing, which is why there are no tests for this in the first place. We could delete bytes from valid files to trigger the errors and commit them as binary blobs. But this is manual work and difficult to update, so it would be better to do this once all the format changes are in. The most important error is "Unsupported remark container version", which is tested by the follow-up patch that bumps the remark version. |
The main goal is to remove the separate mode, and add more features to the bitstream remark format. This patch primarily moves shared helper methods into |
Committing more binary blobs is not a great idea given the |
jroelofs
reviewed
Sep 11, 2025
jroelofs
reviewed
Sep 11, 2025
jroelofs
reviewed
Sep 11, 2025
jroelofs
reviewed
Sep 11, 2025
jroelofs
reviewed
Sep 11, 2025
jroelofs
reviewed
Sep 11, 2025
jroelofs
reviewed
Sep 11, 2025
jroelofs
approved these changes
Sep 11, 2025
Created using spr 1.3.7-wip [skip ci]
tobias-stadler
changed the base branch from
users/tobias-stadler/spr/main.remarks-bitstreamremarkparser-refactor-error-handling
to
main
tobias-stadler
deleted the
users/tobias-stadler/spr/remarks-bitstreamremarkparser-refactor-error-handling
branch
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/10/builds/13493 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/117/builds/13369 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/35442 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/140/builds/30734 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/116/builds/18343 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/123/builds/27173 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/88/builds/16086 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/181/builds/27874 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/205/builds/22021 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/204/builds/22044 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/203/builds/23232 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/33/builds/23034 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/160/builds/24881 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/180/builds/25021 Here is the relevant piece of the build log for the reference |
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Sep 16, 2025
…#156511) In preparation of larger changes to the bitstream remark format, refactor the error handling code in the BitstreamRemarkParser. Main change: move the various static helper methods into the parser helper classes, so we don't need to pass around as many args. Calling `error(...)` inside the helper classes now automatically prepends the current block being parsed to the error message. NFCI (except for error messages on invalid bitstream files). Pull Request: llvm/llvm-project#156511
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Sep 16, 2025
…ndling" (#158647) Reverts llvm/llvm-project#156511. Build failure not caught by pre-commit CI.
tobias-stadler
added a commit
that referenced
this pull request
Sep 16, 2025
…58667) Reland #156511 after fixing a build failure not caught by clang. The default implementation of `parseRecord` is currently unused. Apparently, clang doesn't type check uninstantiated methods in class templates. To avoid this footgun, we `= delete` the impl for now. Original message: In preparation of larger changes to the bitstream remark format, refactor the error handling code in the BitstreamRemarkParser. Main change: move the various static helper methods into the parser helper classes, so we don't need to pass around as many args. Calling `error(...)` inside the helper classes now automatically prepends the current block being parsed to the error message. NFCI (except for error messages on invalid bitstream files). Pull Request: #158667
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In preparation of larger changes to the bitstream remark format,
refactor the error handling code in the BitstreamRemarkParser.
Main change: move the various static helper methods into the parser
helper classes, so we don't need to pass around as many args. Calling
error(...)inside the helper classes now automatically prepends thecurrent block being parsed to the error message.
NFCI (except for error messages on invalid bitstream files).