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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht accepted this revision.
rupprecht marked an inline comment as done.
rupprecht added a comment.

LGTM just with the naming nit from James (header_t&->const HeaderType&)



================
Comment at: llvm/unittests/Object/ObjectFileTest.cpp:83
+  ASSERT_TRUE(!!ObjFileOrErr) << "invalid object file format";
+  ObjPtr = std::move(ObjFileOrErr.get());
+}
----------------
abrachet wrote:
> rupprecht wrote:
> > This should just be returned, i.e. the method should have a signature:
> > 
> > ```
> > template <typename HeaderType>
> > std::unique_ptr<ObjectFile> createObjectFile(const HeaderType &Header, file_magic Magic = file_magic::unknown)
> > ```
> > 
> > (+1 to using the name suggestion from James)
> If I returned the unique_ptr then I couldn’t have the ASSERT_TRUE which returns void on error. I would like to keep it, but it isn’t totally necessary I suppose.
Oh, yeah. I knew there was a weird gotcha with `ASSERT_TRUE`, but couldn't remember specifically. :(

Using a test fixture would workaround this issue; e.g. the test class would hold an empty std::unique_pt<ObjectFile> ObjPtr that the test method would initialize (and would not assign as an out-parameter as you have here). But I think this is fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62838