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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
