[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>
+
----------------
abrachet wrote:
> 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 
I agree with basically everything you just said, but ultimately I'm going on what common practice seems to be. I really don't care that much, and would happily back bringing this up on the mailing list as an inconsistency in how the coding standard is applied etc, with a suggestion to either change the standard or seeking agreement to start pushing more concretely for it.

> Also, if one of those files no longer needs vector then the build breaks.
This is not really an issue to worry about, since people should build their code before committing anyway.


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


================
Comment at: llvm/unittests/Object/ObjectFileTest.cpp:109
+  // They don't make a copy, so there's no reason to call
+  // ObjectFile::createObjectFile every time
+  for (auto CurrType : PossibleTypes) {
----------------
Missing trailing full stop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62838