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

Wed Jun 12 10:29:07 PDT 2019

abrachet marked an inline comment as done.
abrachet added inline comments.

================
Comment at: llvm/unittests/Object/ObjectFileTest.cpp:17-18
+
+#include <functional>
+#include <vector>
+
----------------
jhenderson wrote:
> 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.
I don’t personally interpret this in the same way you do. It seems to me that is mainly targeting when a forward declaring would suffice which is not the case here. I’m sure that one of the headers that I have included itself includes vector, but the compile time performance cost mentioned in that standard I would guess isn’t concerned with the cost of an ifdef. Also, if one of those files no longer needs vector then the build breaks. I wrote a lot more than is representative of how much I care. I’m not putting my foot down or anything, it’ll gladly remove the include if you still want 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62838