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

Tue Jun 11 02:58:30 PDT 2019

jhenderson added inline comments.

================
Comment at: llvm/unittests/Object/ObjectFileTest.cpp:17-18
+
+#include <functional>
+#include <vector>
+
----------------
abrachet wrote:
> 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?
See https://llvm.org/docs/CodingStandards.html#include-as-little-as-possible. So, if one of the headers includes them, my understanding is that you don't need to explicitly mention them in the source too.

================
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));
----------------
abrachet wrote:
> 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.
Yes, exactly, you don't need this block, because you are already testing it above. I don't think you need a separate test that does the same thing.

Sometimes a little copy-pasting might make it easier to read, just be careful how much. In this case, I don't think you need to.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62838