[PATCH] D87749: [SyntaxTree][Synthesis] Implement `deepCopy`

[Eduardo Caldas via Phabricator via cfe-commits]

eduucaldas added inline comments.


================
Comment at: clang/lib/Tooling/Syntax/Synthesis.cpp:237
+
+syntax::Node *clang::syntax::deepCopy(syntax::Arena &A, const Node *N) {
+  if (!canModifyAllDescendants(N))
----------------
We are ignoring nullability of pointers.

The casting machinery asserts that on `dyn_cast` we don't have `nullptr`. So this code crashes on `nullptr`. 

>From what I understand `dyn_cast` et al. are intended to be used on pointers. Are there other alternatives to this approach? I would like to encode the non-nullability in types instead of in asserts....


================
Comment at: clang/lib/Tooling/Syntax/Synthesis.cpp:205
+  if (L->canModify())
+    syntax::FactoryImpl::setCanModify(Leaf);
+
----------------
gribozavr2 wrote:
> Since we are creating new leaves, why prohibit their mutation sometimes?
> 
> I also don't quite understand the implications of having multiple leaves in a tree that are backed by the same token. I think the algorithm that produces edits can be confused by that.
> 
> If you agree, please change the implementation to use `createLeaf` (or call it directly from `deepCopy`).
> Since we are creating new leaves, why prohibit their mutation sometimes?

The `canModify` says whether the leaf is backed by a macro.

Since the tokens coming from macros are expanded they would be expanded in the deepCopy as well. We agreed that this shouldn't be the default behaviour. We can have `deepCopyExpandingMacros`, if the user wants this behaviour, but I think `deepCopy` should simply forbid macros.

WDYT about `deepCopyExpandingMacros`?

> having multiple leaves in a tree that are backed by the same token

We think the current algorithm wouldn't be confused by that.
However it's easier to reason about `Leaf`s each with their `Token`, and we don't think creating additional `Leaf` nodes would affect performance largely.

So we'll call `createLeaf` instead to create a fresh Leaf with the same Token as the previous one.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87749/new/

https://reviews.llvm.org/D87749