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

[Andy Kaylor via Phabricator via llvm-commits]

andrew.w.kaylor added inline comments.


================
Comment at: include/llvm/IR/LLVMContext.h:324
+  /// analysis.
+  void setOptPassGate(OptPassGate&);
+
----------------
fedor.sergeev wrote:
> andrew.w.kaylor wrote:
> > yrouban wrote:
> > > 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.
> > > 
> > My concern is lifetime management. If the LLVMContext receives a reference to an object, the lifetime of that object must be guaranteed to extend as long as the lifetime of the LLVMContext. That was always true with the OptBisect object, but with an arbitrary input from any caller we can't be sure of that. This feels like a weak spot in the interface.
> This is definitely a weak spot in the interface, which can not be addressed without a heavy redesign of the whole approach, which borders pretty much on throwing everything out and coming out with a completely new design.
> 
> Will a comment on the interface that explicitly mentions that management of the lifetime is at discretion of interface user satisfy your concerns?
Yes, given the limited expected usage of this interface I would be satisfied with just a comment.


https://reviews.llvm.org/D44464