This is an archive of the discontinued LLVM Phabricator instance.

[SyntaxTree][Synthesis] Implement `deepCopy`
ClosedPublic

Authored by eduucaldas on Sep 16 2020, 1:51 AM.

Diff Detail

Event Timeline

eduucaldas created this revision.Sep 16 2020, 1:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2020, 1:51 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
eduucaldas requested review of this revision.Sep 16 2020, 1:51 AM

Remove extraneous qualifiers in createTree

gribozavr2 accepted this revision.Sep 17 2020, 12:47 PM
gribozavr2 added inline comments.
clang/include/clang/Tooling/Syntax/BuildTree.h
45

"Creates a completely independent copy of N (a deep copy)."

clang/lib/Tooling/Syntax/Synthesis.cpp
208

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).

clang/unittests/Tooling/Syntax/SynthesisTest.cpp
166

Copy => DeepCopy? (also in other tests)

This revision is now accepted and ready to land.Sep 17 2020, 12:47 PM
eduucaldas marked 3 inline comments as done.
  • deepCopy returns nullptr if copy code with Macro expansions.
  • deepCopy also deep copies the Tokens.
gribozavr2 added inline comments.Sep 21 2020, 12:26 AM
clang/lib/Tooling/Syntax/Synthesis.cpp
216

Shouldn't we call canModifyAllDescendants recursively?

228
239

Could you add tests for deepCopy returning nullptr?

eduucaldas added inline comments.Sep 21 2020, 12:49 AM
clang/lib/Tooling/Syntax/Synthesis.cpp
208

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 Leafs 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.

237

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....

eduucaldas marked 3 inline comments as done.

Fix canModifyAllDescendants, add tests for it

This revision was landed with ongoing or failed builds.Sep 21 2020, 2:28 AM
This revision was automatically updated to reflect the committed changes.
gribozavr2 added inline comments.Sep 21 2020, 2:56 AM
clang/lib/Tooling/Syntax/Synthesis.cpp
208

WDYT about deepCopyExpandingMacros?

That could be a useful operation that we can provide.

237

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.

clang/unittests/Tooling/Syntax/SynthesisTest.cpp
220

Does EXPECT_EQ work?