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