jansvoboda11 added a comment. In D99568#2659526 <https://reviews.llvm.org/D99568#2659526>, @dexonsmith wrote:
> I'm not sure a deep copy is entirely sound, due to odd ownership rules in > "remapped buffers" (I forget the subclass that includes those). A few months > ago I got most of the way to deleting those (lifting the logic up into the > clang tooling that needed it)... maybe we need to push that work over the > line before this is safe... or if this is in fact safe, can you explain why? Ah, I didn't notice those. I think you may be referring to `PreprocessorOptions::RemappedFileBuffers`. I agree that the raw `MemoryBuffer` pointers don't play well with the concept of deep copy, so getting that sorted would be good. How about we split this patch into two? 1. Changes to `AnalyzerOptions`. They make the current implementation of deep copy constructor more correct, so we can commit them right away. 2. Addition of copy assignment. It increases the visibility/surface area of the deep copy operation, so we can commit that when `RemappedFileBuffers` get fixed. In the meantime, I can work around its absence in my next patch. WDYT? Note: I looked at the members of `CompilerInvocation` and noticed one other field that's a reference type: `PreprocessorOptions::FailedModules`, which acts as a shared state, not so much as an actual option. I don't think it creates lifetime issues like `RemappedFileBuffers`. Although sharing it across deep copies of `CompilerInvocation` may be a bit unexpected, that seems to be the current invariant. Extracting that out of `PreprocessorOptions` would be nice though. > BTW, I may be responsible for the not-very-descriptive naming of > `CompilerInvocationBase`... I could imagine new options coming along that get > added to the wrong place in the future as well. Any thoughts on a good name > to use instead? We could make the split obvious by renaming `CompilerInvocationBase` to `CompilerInvocationBaseReferenceType`, renaming `CompilerInvocation` to `CompilerInvocationBaseValueType` and creating new `CompilerInvocation` class that inherits from both. I'm not sure if we have any precedent for this pattern. I'll look around. ================ Comment at: clang/include/clang/Frontend/CompilerInvocation.h:86 CompilerInvocationBase(const CompilerInvocationBase &X); - CompilerInvocationBase &operator=(const CompilerInvocationBase &) = delete; + CompilerInvocationBase &operator=(CompilerInvocationBase X); ~CompilerInvocationBase(); ---------------- dexonsmith wrote: > I wonder if it would be better to signify this is a deep copy by naming the > function, such as by calling it `clone()`. WDYT? We can be more explicit about that. In that case, it would probably make sense to delete the copy constructor to make `clone()` the only way to get a deep copy. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99568/new/ https://reviews.llvm.org/D99568 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
