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

[Alex Brachet via Phabricator via llvm-commits]

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


================
Comment at: llvm/unittests/Object/ObjectFileTest.cpp:17-18
+
+#include <functional>
+#include <vector>
+
----------------
jhenderson wrote:
> Do you need both these headers? Note that in LLVM, the standard is to only include the headers that are actually required.
I do use both std::function and std::vector. I suppose that std::vector was not 100% required, but it makes things somewhat cleaner I would say. Would you have a strong preference that I take it out?


================
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));
----------------
jhenderson wrote:
> Sorry, I don't understand this?
I probably over complicated things with this entire function. A lot of work to not copy and paste. The idea behind these lines was that although most likely it would be tested in the upper loop, this explicitly tests for example if a ObjectFile::isExecutableObject() is true when ET_EXEC is set as its type. I say this isn't mandatory because ET_EXEC would have already been tested going through the loop on 112 as it is in ELF_ETypes.


================
Comment at: llvm/unittests/Object/ObjectFileTest.cpp:179
+                                                     file_magic::macho_object);
+    ASSERT_TRUE(!!ObjFileOrErr);
+    if (Type == MachO::MH_EXECUTE)
----------------
jhenderson wrote:
> Why the double '!'? Does ASSERT_TRUE not work directly on ObjectFileOrErr?
Seems like gtest doesn't play well with operator bool? I'm not really sure but it didn't work for me without the manual bool conversion. They explicitly mention this in the ErrorOr unit tests, actually: https://github.com/llvm/llvm-project/blob/master/llvm/unittests/Support/ErrorOrTest.cpp#L23


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62838