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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added inline comments.


================
Comment at: clang/lib/Tooling/Syntax/Synthesis.cpp:205
+  if (L->canModify())
+    syntax::FactoryImpl::setCanModify(Leaf);
+
----------------
eduucaldas wrote:
> 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.
> WDYT about deepCopyExpandingMacros?

That could be a useful operation that we can provide.


================
Comment at: clang/lib/Tooling/Syntax/Synthesis.cpp:237
+
+syntax::Node *clang::syntax::deepCopy(syntax::Arena &A, const Node *N) {
+  if (!canModifyAllDescendants(N))
----------------
eduucaldas wrote:
> 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....
I don't think it is idiomatic in the LLVM/Clang style to use references for AST nodes and similar things. Beyond that, there is the issue that references are non-rebindable.


================
Comment at: clang/unittests/Tooling/Syntax/SynthesisTest.cpp:220
+  auto *Copy = deepCopy(*Arena, OriginalTree);
+  EXPECT_TRUE(Copy == nullptr);
+}
----------------
Does EXPECT_EQ work?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87749