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

[Alex Brachet via Phabricator via llvm-commits]

abrachet marked 11 inline comments as done.
abrachet added inline comments.


================
Comment at: llvm/unittests/Object/ObjectFileTest.cpp:48-51
+    ELF::ET_LOPROC + 20,
+    ELF::ET_LOPROC + 38,
+    ELF::ET_LOPROC + 50,
+    ELF::ET_LOPROC + 72,
----------------
jhenderson wrote:
> abrachet wrote:
> > jhenderson wrote:
> > > You probably only need one of these, but you should also add a value between LOOS and HIOS.
> > > 
> > > You should probably even add an unused value from the standard range (e.g. ET_CORE + 1), or similar.
> > Which LOOS and HIOS should I use in the context of e_type?
> Any arbitrary value between ET_LOOS and ET_HIOS.
> 
> Reference in case you need it: http://www.sco.com/developers/gabi/latest/ch4.eheader.html
Thanks, I was confused because these values were not in BinaryFormat/ELF.h, I have added them.


================
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) {
----------------
rupprecht wrote:
> 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).
Yup the classic saved a few minutes of copy and pasting for a couple of hours fighting the compiler. You should have seen how bad it was before :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62838