[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:17-18
+
+#include <functional>
+#include <vector>
+
----------------
Do you need both these headers? Note that in LLVM, the standard is to only include the headers that are actually required.


================
Comment at: llvm/unittests/Object/ObjectFileTest.cpp:47
+    ELF::ET_CORE,
+    // these are arbitarily chosen between LOPROC and HIPROC.
+    ELF::ET_LOPROC + 20,
----------------
Nit: capital letter at start of comment.


================
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,
----------------
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.


================
Comment at: llvm/unittests/Object/ObjectFileTest.cpp:109
+
+  // COFFObjectFile and ELFObjectFile just use the buffer directly, 
+  // it doesn't copy so theres no reason to call 
----------------
',' -> '.'


================
Comment at: llvm/unittests/Object/ObjectFileTest.cpp:110
+  // COFFObjectFile and ELFObjectFile just use the buffer directly, 
+  // it doesn't copy so theres no reason to call 
+  // ObjectFile::createObjectFile every time, this is enough.
----------------
Change the rest of the comment to:
"They don't make a copy, so there's no reason to call ObjectFile::createObjectFile every time."


================
Comment at: llvm/unittests/Object/ObjectFileTest.cpp:120-123
+  // Already would have been tested but feels more comfortable to explicitly
+  // test what we are testing.
+  SetType(Header, Type);
+  EXPECT_TRUE(TestMethod(ObjFile));
----------------
Sorry, I don't understand this?


================
Comment at: llvm/unittests/Object/ObjectFileTest.cpp:162-163
+// A test for isRelocatableObject for COFF is notably missing because
+// this isn't identified in the header for COFF and is more involved, not what
+// testFileType can do.
+
----------------
Delete the bit after the last comma.


================
Comment at: llvm/unittests/Object/ObjectFileTest.cpp:165
+
+// MachO are tested differently because they are constructed differently
+// parseHeader uses memcpy so we can't test in the same way that we do with
----------------
Add "files" after "MachO". Add a full stop at the end of this line.


================
Comment at: llvm/unittests/Object/ObjectFileTest.cpp:167-168
+// parseHeader uses memcpy so we can't test in the same way that we do with
+// COFF and ELF which don't do this. It made testFileType too ugly to accomadate
+// this.
+TEST(IsExecutableObject, MachO) {
----------------
Delete the last sentence. I don't think it needs explaining in the test.


================
Comment at: llvm/unittests/Object/ObjectFileTest.cpp:179
+                                                     file_magic::macho_object);
+    ASSERT_TRUE(!!ObjFileOrErr);
+    if (Type == MachO::MH_EXECUTE)
----------------
Why the double '!'? Does ASSERT_TRUE not work directly on ObjectFileOrErr?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62838