[PATCH] D44464: allow custom OptBisect classes set to LLVMContext

[Yevgeny Rouban via Phabricator via llvm-commits]

yrouban added inline comments.


================
Comment at: include/llvm/IR/LLVMContext.h:324
+  /// analysis.
+  void setOptPassGate(OptPassGate&);
+
----------------
andrew.w.kaylor wrote:
> It seems like a bad idea to pass this in by reference. How is the lifetime of this object going to be managed? I don't think it's correct for the LLVMContext to have a reference to anything outside the LLVMContext. If you're going to pass in an object, I think LLVMContext must take ownership of the object, as happens with LLVMContext::setDiagnosticHandler().
Andy, can you please explain why LLVMContext should not have a ref to outside? The OptBisect, for example, is one single global object that is outside of LLVMContext.
What if a user wants to set one OptPassGate to several contexts?
I would suggest that we accept this minimal design and see if there will be any demand for ownership.



================
Comment at: lib/IR/LLVMContextImpl.h:1362
   /// analysis.
   OptPassGate &getOptPassGate();
+
----------------
andrew.w.kaylor wrote:
> I missed this in the last review. This function should be const.
D44821 did not change this. LLVMContextImpl::getOptBisect() used to be non-const.



https://reviews.llvm.org/D44464