[PATCH] D62838: [Object] add isExecutableObject member function

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/unittests/Object/ObjectFileTest.cpp:47
+class ELFTest : public TestBase<ELF64LE::Ehdr> {
+  void createGenericHeader() override {
+    memset(&Header, 0, sizeof(Header));
----------------
You've marked this as `private` (implicitly) here, but it's `protected` in the base class. Is that intended?


================
Comment at: llvm/unittests/Object/ObjectFileTest.cpp:63
+
+protected:
+  ELFTest() {
----------------
Do you really need to make the constructor `protected`?


================
Comment at: llvm/unittests/Object/ObjectFileTest.cpp:152
+
+TEST_F(MachOTest, IsRelocatable) {
+  for (auto Type : Types) {
----------------
I wonder if it would make more sense to fold this into the test case above? i.e. the check becomes:

```
    if (Type == MachO::MH_EXECUTE) {
      EXPECT_TRUE(ObjPtr->isExecutableObject());
      EXPECT_FALSE(ObjPtr->isRelocatableObject());
    } else if (Type == MachO::MH_OBJECT) {
      EXPECT_FALSE(ObjPtr->isExecutableObject());
      EXPECT_TRUE(ObjPtr->isRelocatableObject());
    } else { // Don't know if this is really necessary or not, but you get the idea
      EXPECT_FALSE(ObjPtr->isExecutableObject());
      EXPECT_FALSE(ObjPtr->isRelocatableObject());
    }
```

On the other hand, if you think the separate test-cases is easier to read, I'm okay with it.

Same comment applies to ELF.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62838