[PATCH] D92923: [llvm-readelf/obj] - Add support of multiple SHT_SYMTAB_SHNDX sections.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/ELF.h:75
+        return createError(
+            "the index is greater or equal to the number of entries (" +
+            Twine(*Size) + ")");
----------------



================
Comment at: llvm/test/tools/llvm-readobj/ELF/symtab-shndx.test:199
+## Check we report a warning when multiple SHT_SYMTAB_SHNDX sections are linked to a symbol table.
+## In this case 2, sections are linked to the dynamic symbol table. Check it doesn't affect
+## anything, because the SHT_SYMTAB_SHNDX section specified by the DT_SYMTAB_SHNDX tag is still used.
----------------
You didn't follow my fix properly here :)


================
Comment at: llvm/unittests/Object/ELFTest.cpp:66-69
+      if (Expected<uint8_t> ValOrErr = R[I])
+        EXPECT_EQ(*ValOrErr, I);
+      else
+        ADD_FAILURE() << toString(ValOrErr.takeError());
----------------
Could this be:

```
ASSERT_THAT_EXPECTED(ValOrErr, Succeeded());
EXPECT_EQ(*ValOrErr, I);
```

Or something to that effect? It seems simpler, and if the good index-value cases don't work, there doesn't seem to be much point in worrying about the rest of the test case running, so asserting is fine.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92923/new/

https://reviews.llvm.org/D92923