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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added inline comments.


================
Comment at: llvm/unittests/Object/ObjectFileTest.cpp:23
+
+static void createGenericELFHeader(ELF64LE::Ehdr &Header) {
+  memset(&Header, 0, sizeof(Header));
----------------
For these create*Header methods, it looks like these can all create their own header variables instead of making the caller pass a blank one in?


================
Comment at: llvm/unittests/Object/ObjectFileTest.cpp:92-117
+template <typename header_t>
+static void testFileType(header_t Header, unsigned Type,
+                         std::function<bool(ObjectFile &)> TestMethod,
+                         std::function<bool(unsigned)> KnownTrue,
+                         std::function<void(header_t &, unsigned)> SetType,
+                         std::vector<unsigned> PossibleTypes,
+                         file_magic magic = file_magic::unknown) {
----------------
This is a bit too clever; it looks great but it makes it harder to read. Tests, even more than code, should be simple to read to make sure the test is actually doing the right thing. Otherwise, if it gets too complex, you have to start writing tests for the test itself :)

I think a good way to make it simpler would be to break it apart, e.g. one that reinterprets to an `ObjectFile`, and break apart the true/false types into different loops (avoiding the need for `KnownTrue`), like:

```
template <typename header_t>
static void createObjectFile(header_t Header, file_magic magic = file_magic::unknown) {
  StringRef Bytes(reinterpret_cast<const char *>(&Header), sizeof(Header));
  auto ObjFileOrErr =
      ObjectFile::createObjectFile(MemoryBufferRef(Bytes, ""), magic);
  ASSERT_TRUE(!!ObjFileOrErr) << "invalid object file format";

  return *ObjFileOrErr.get();
}

TEST(IsExecutableObject, ELF) {
  ELF64LE::EHdr Header = createGenericELFHeader();
  ObjectFile Obj = createObjectFile(Header);
  for (auto ElfType : { ELF::ET_NONE, ELF::ET_REL, ... }) {
    Obj.Header = ElfType;
    EXPECT_FALSE(Obj.isExecutable());
  }
  // Same for EXPECT_TRUE
}
```

Depending on how you refactor, you might also want to put these into a test fixture (i.e. create a test class and use `TEST_F`), though I'm not sure there's any data to share (the usual reason for creating a test fixture).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62838