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

[Alex Brachet via Phabricator via llvm-commits]

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


================
Comment at: llvm/unittests/Object/ObjectFileTest.cpp:83
+  ASSERT_TRUE(!!ObjFileOrErr) << "invalid object file format";
+  ObjPtr = std::move(ObjFileOrErr.get());
+}
----------------
rupprecht wrote:
> 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.
How does this look now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62838