Lunderberg commented on PR #12822: URL: https://github.com/apache/tvm/pull/12822#issuecomment-1251472206
> Would be great to think about we really want to enable the case of vector<With>. I had found it useful for cases where I had `N` expressions, and I wanted to simplify one of them under the condition that the other `N-1` of them are true. The `vector<With>` felt like the cleaner solution, as compared to constructing an `And` of the `N-1` conditions, then destructuring it back into `N-1` conditions on the other side. > Specifically, the default constructor(that initialize optional as nullopt) can be confusing for people who intended to start and use it. I had been seeing it as a default for cases where a context may or may not be required. That said, now that we have C++17 we could instead use `std::optional<With<T>>`, so that use case isn't relevant anymore. > With is originally designed to only as a sugar to enter a context and exit with RAII but not intended to put them into a vector. This makes sense, though in that case I think we should disable both the copy and move constructor. > Perhaps we can simply disable the copy constructor in this case and make move constructor available only if the underlying Context itself is movable. This would work for classes that already function entirely as context managers (e.g. `arith::ConstraintContext`), though in those cases the `With` wrapper becomes unnecessary. For classes where the class serves as both a data type and as a context (e.g. `tvm::Target`), I think we need to distinguish between whether the context can be moved/copied, and whether the context manager can be moved/copied. > Finally, we can find alternatives when maintaining multiple such Constraint. E.g. a different constraint context that takes multiple such constraints. For cases where the order of exiting a context must be the reverse of entering the context, I agree. For cases where the order of exiting the context is arbitrary, but where several destructors should all be held and called at a later point, that's already the behavior of `std::vector<ContextManager>`, so I don't think we'd need to recreate it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
