[PATCH] D149522: [JITLink] Add target features to LinkGraph

[Job Noorman via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/ExecutionEngine/JITLink/COFFLinkGraphBuilder.cpp:32
+          Obj.getFileName().str(), createTripleWithCOFFFormat(TT),
+          cantFail(Obj.getFeatures()).getFeatures(), getPointerSize(Obj),
+          getEndianness(Obj), std::move(GetEdgeKindName))) {
----------------
lhames wrote:
> I think I missed this the first time around. Why `cantFail`? There should be a comment explaining why, at this particular call-site, it would be impossible for this call to ever fail. If it could fail then the API needs to be refactored to return an `Error` / `Expected` (in the case of constructors this usually means switching to a named constructor idiom, or you could check/retrieve the FeatureVector in the caller, then pass it into this constructor).
You're right, this use of `cantFail` wasn't appropriate. I convinced myself after a cursory glance that `getFeatures` never fails for any architecture and that `cantFail` would therefore be an easy solution. This isn't true, however, so I've updated the patch to propagate errors.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149522