[PATCH] D102296: [ELF] getRelocatedSection: remove the check for ET_REL object file

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:594-595
+
+// Test for ObjectFile::getRelocatedSection: check that it returns relocated
+// section for executable file.
+TEST(ELFObjectFileTest, ExecutableWithRelocs) {
----------------
Whilst you're adding this test, it probably makes sense for one for ET_REL files too, since you're explicitly calling out the executable version.


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:601
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
----------------
Tip: you don't need all this whitespace. Just get rid of the excess, so that it nicely lines up, but without a massive gap.


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:613-619
+  - Type:            SectionHeaderTable
+    Sections:
+      - Name:            .text
+      - Name:            .rela.text
+      - Name:            .symtab
+      - Name:            .strtab
+      - Name:            .shstrtab
----------------
This looks to be unnecessary. The `SectionHeaderTable` block is only needed if you want to have a different order for sections in the file versus in the section header table.


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:643
+    ASSERT_NE(RelSec, Obj.section_end());
+    ASSERT_TRUE(RelSec->isText());
+    Expected<StringRef> TextSecNameOrErr = RelSec->getName();
----------------
The `ASSERT_TRUE` and `ASSERT_EQ` here and below can probably all be `EXPECT_TRUE` and `EXPECT_EQ` - it's harmless for the test to continue to run in these cases.

I don't think you actually need the `RelSec->isText()` check anyway. Really, all that's important is that the section name is correct, since there's only one ".text" section.


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:650
+  }
+  ASSERT_TRUE(FoundText);
+  ASSERT_TRUE(FoundRela);
----------------
Do you need this? `FoundRela` seems like it'd be enough - one of the other checks will fail otherwise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102296