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

[Lang Hames via Phabricator via llvm-commits]

lhames added a comment.

The `cantFail`s should be resolved (either commented if they truly can't fail, or removed if they could), but otherwise LGTM.



================
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))) {
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149522