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.