Details
- Reviewers
jhenderson shchenz Higuoxing DiggerLin - Group Reviewers
Restricted Project - Commits
- rGb28e8abfd069: [NFC][XCOFF][llvm-readobj] replace binaries with YAMLs (only tests for Symbols).
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
A patch like this should be marked with "NFC" in the title
llvm/test/tools/llvm-readobj/XCOFF/symbols64.test | ||
---|---|---|
94 | Maybe use a meaningful date here? | |
100 | We don't need to mention IBM XLC V16.1 now. Please use IBM Open XL instead. | |
107 | whitespace at the end? | |
115 | The length seems not right to me. | |
134 | Same as above | |
153 | Same as above |
Mostly looks good, but I think we can simplify the test a little whilst we're here, as suggested in a couple of places inline.
My comments in the two tests apply to both tests, so please address in both cases/respond for both cases.
llvm/test/tools/llvm-readobj/XCOFF/symbols-invalid.test | ||
---|---|---|
5–6 | I'm not necessarily opposed to this being a separate file, although I think it's more common in newer tests to keep invalid behaviour checks in the same files as their corresponding regular behaviour. Also the %t5 needs changing to just %t. | |
llvm/test/tools/llvm-readobj/XCOFF/symbols.test | ||
30 | Should this be CSECT rather than SECT? | |
90 | The addition of the # characters has made this a little hard to review. Are there any actual changes in the checks performed, or is the output semantically identical? | |
llvm/test/tools/llvm-readobj/XCOFF/symbols64.test | ||
88 | The 8 sounds important. I'd change to just test.c | |
94 | More preceisely, as this is just a free string in the YAML, let's just change it to something more obviously arbitrary e.g. "foo" | |
100 | Does the test actually need this many auxiliary entries to exercise the llvm-readobj behaviour appropriately? If so, as above, just use an arbitrary string here e.g. "bar". |
Address comments.
llvm/test/tools/llvm-readobj/XCOFF/symbols-invalid.test | ||
---|---|---|
5–6 | Thanks for your comments! | |
llvm/test/tools/llvm-readobj/XCOFF/symbols.test | ||
30 | It should be Section auxiliary entry, and abbreviated as SECT auxiliary entry. | |
90 | I removed duplicated checks for a given symbol type and added a check for C_WEAKEXT that were not tested. | |
llvm/test/tools/llvm-readobj/XCOFF/symbols64.test | ||
115 | The length value is correct in fact. |
LGTM too. Thanks for cleaning up.
llvm/test/tools/llvm-readobj/XCOFF/symbols64.test | ||
---|---|---|
115 | Ah, OK. I didn't notice that this big length is coming from the YAML. Thanks for the explanation. |
I'm not necessarily opposed to this being a separate file, although I think it's more common in newer tests to keep invalid behaviour checks in the same files as their corresponding regular behaviour.
Also the %t5 needs changing to just %t.