[PATCH] D99568: [clang][invocation] Fix copy constructor, add copy assignment to CompilerInvocation

[Jan Svoboda via Phabricator via cfe-commits]

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