erichkeane wrote: > > Can you share what the Static assert complaint is that makes you think we > > should be doing this? > > The complaint is that `Preprocessor::addPragmaHandler` called from parser > with a raw ptr argument coming from a `unique_ptr` owned by Parser actually > initializes another `unique_ptr` with that same raw ptr (the second > `unique_ptr` is stored in a `PragmaNamespace` which is owned by > `Preprocessor`). That results in two `unique_ptr`s that manage the same > memory at the same time. Which seems dangerous. This also creates a > requirement to not forget to call `removePragmaHandler` at the right time, > otherwise the two `unique_ptr`s may cause double free.
This is definitely concerning. But because we're properly managing lifetime, it seems sensible to me to just have `PragmaNamespace` actually just store a pointer or reference to the `unique_ptr` instead. > It seems I can't get rid of PragmaHandlers in Parser, since some of the > handlers are "added" to Preprocessor under different namespaces using the > same pointer owned by Parser (example UnrollHintHandler). It seems I can't > remove a smart pointer from `PragmaNamespace` either since Preprocessor > itself adds some handlers via `addPragmaHandler` (see > `Preprocessor::RegisterBuiltinPragmas`) which are not stored anywhere. The > two `unique_ptr`s created for the same raw pointer makes me think it should > be a `shared_ptr` instead. The problem with `shared_ptr` is it creates a separation of ownership that we don't really mean. As you mentioned (and I can see now), we're conflating ownership in a poor way. However, `shared_ptr` instead makes it so that the consequence of forgetting to unregister (which, IMO, is part of our ownership model) is now `fine` because it gets destroyed with whoever remembers to undo it last. At least before we were warned about it by a double-free during runtime. I would vastly prefer the destructor of the thing that needs to have `removePragmaHandler` assert if it hasn't been unregistered, then leave it as a `unique_ptr` in the Parser. https://github.com/llvm/llvm-project/pull/117703 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
