Details
Diff Detail
Event Timeline
llvm/test/tools/llvm-readobj/ELF/reloc-addends.test | ||
---|---|---|
2 | dumps -> dump | |
4 | Inconsistent spacing for -D option (should be -DENCODE=LSB or -D TYPE=SHT_RELA). Same throughout. | |
9–10 | This test is about addend printing, so we probably don't need the scoping/header lines for context. Same goes throughout. You also probably don't need --match-full-lines. | |
11 | Here and on all the lines, you probably want R_X86_64_NONE - 0x0{{$}} instead of --match-full-lines, as you don't need the offset for the test. | |
25–26 | Unless you have a patch to imminently fix this, I'd recommend filing a bug. Same goes for the other FIXMEs below (and indeed all FIXMEs in general). | |
34 | I think I'd find it slightly easier to read this test if identical readelf and readobj test cases are grouped together. In other words: # RUN: llvm-readobj <little endian, rela> # RUN: llvm-readelf <little endian, rela> # checks for little endian rela # RUN: llvm-readobj <big endian, rela> # RUN: llvm-readelf <big endian, rela> # checks for big endian rela # RUN: llvm-readobj <little endian, rel> # RUN: llvm-readelf <little endian, rel> # checks for little endian rel # RUN: llvm-readobj <big endian, rel> # RUN: llvm-readelf <big endian, rel> # checks for big endian rel | |
39 | Don't need this line. | |
40 | You only really need the Type to Addend columns in this line. | |
83 | Any particular reason you have an arbitrary negative number, but not an arbitrary positive one? | |
87 | Is there anything actually stopping us using R_X86_64 for both 64 and 32 bit cases, and therefore allowing us to use the same YAML throughout? |
- Addressed review comments.
llvm/test/tools/llvm-readobj/ELF/reloc-addends.test | ||
---|---|---|
25–26 | I've added a link to the existent bug here and reported a new bug and added a link for a FIXME below. | |
83 | Nope. I just do not like the negative numbers representation I think, my brain assumes them are "special". | |
87 | 32 and 64 bit cases not only have different relocations, but also the different size of r_addend. |
llvm/test/tools/llvm-readobj/ELF/reloc-addends.test | ||
---|---|---|
83 | I.e -> I've |
LGTM, with a couple of nits.
llvm/test/tools/llvm-readobj/ELF/reloc-addends.test | ||
---|---|---|
14 | Missing {{$}}? | |
18 | I'd probably delete this blank line, to show that this is tied to the GNU block (mostly for consistency with my comment in the next section). | |
36–38 | The FIXME is tied to the LLVM case, right? If so, delete the blank line after it. At the moment, it looks like it applies to both cases. | |
97 | Same comment as above re. the blank line after this FIXME. | |
117 | Same comment as above re. this FIXME. |
dumps -> dump
rellocations -> relocations