Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| llvm/test/tools/llvm-objdump/malformed-archives.test | ||
|---|---|---|
| 1–3 | ||
| 2 | ||
| 5–6 | I'd add comments before each test case to make it clear what it is testing. | |
| 6–7 | Optional nit, here and below. | |
| 10–14 | Looking at the original libbogus1.a and libbogus2.a, I think the test was testing one or both of two possible things:
I think the second point, in particular the bit about the offset being correct, is probably somewhat useful. | |
| 31 | A comment showing the structure here might be good. Alternatively, what happens if you change the Terminator to be something shorter? Would that be sufficient to produce the same failure? | |
| 38–40 | The order earlier in the file is RUN/YAML/message, but it's switched here. I don't really mind which way around it is, but it should at least be consistent. | |
| 48–79 | Aside: Not really sure this should be an error. | |
| 177–180 | Aside: This test has nothing to do with llvm-objdump. Probably wants moving into the Object or tools/llvm-ar testing. | |
- Addressed review comments.
- Rebased after update of D89949.
| llvm/test/tools/llvm-objdump/malformed-archives.test | ||
|---|---|---|
| 10–14 | I've restored that case. | |
| 31 |
All fields, including the "Terminator" are filled to their expected length with the 0x20 (space) character. I've made the sequence a bit shorter though. | |
| 48–79 | I don't know either. But note that this only happens for BSD and DARWIN64: if (Kind == Archive::K_BSD || Kind == Archive::K_DARWIN64) {
if (ArMemHdr->Name[0] == ' ') {
uint64_t Offset = reinterpret_cast<const char *>(ArMemHdr) -
Parent->getData().data();
return malformedError("name contains a leading space for archive member "
"header at offset " + Twine(Offset));
... | |
| 177–180 | True. | |
| llvm/test/tools/llvm-objdump/malformed-archives.test | ||
|---|---|---|
| 6 | "and vice versa" implies the second case is valid (whilst the first isn't), which isn't actually true. | |
| 10 | Nit. Same below. | |
| 14–16 | Here and throughout, we should probably check the name in the error, message, right? | |
| 45–46 | I had trouble understanding the original comment, so I've suggested an alternative. | |
| 48–79 | ||
| 74 | ||
| 87 | ||
| 125 | ||
| 138 | ||
| 151 | ||
| 164 | ||
- Addressed review comments.
| llvm/test/tools/llvm-objdump/malformed-archives.test | ||
|---|---|---|
| 45–46 | Thanks. | |