[PATCH] D64573: [Syntax] Allow to mutate syntax trees

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 accepted this revision.
gribozavr2 added a comment.
This revision is now accepted and ready to land.

Generally looks good, just nitpicks.



================
Comment at: clang/include/clang/Tooling/Syntax/BuildTree.h:24
                                          const clang::TranslationUnitDecl &TU);
+/// Allows to create syntax trees from subtrees not backed by the source code.
+class Factory {
----------------
"Creates syntax trees..."


================
Comment at: clang/include/clang/Tooling/Syntax/BuildTree.h:25
+/// Allows to create syntax trees from subtrees not backed by the source code.
+class Factory {
+public:
----------------
Why a class to contain static functions? Is it like a namespace, or is there some future design intent?..

Or is it for the friendship?


================
Comment at: clang/include/clang/Tooling/Syntax/Mutations.h:26
+
+/// Either removes a statement or replaces it with an empty statement.
+/// EXPECTS: S->canModify() == true
----------------
Removes a statement, or replaces it with an empty statement where one is required syntactically.

(provide an explanation why it may sometimes use an empty statement -- I think it is not obvious)


================
Comment at: clang/include/clang/Tooling/Syntax/Mutations.h:33
+
+#endif
----------------
Please add the newline.


================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:87
+  bool isDetached() const;
+  /// Whether the node was created from the AST backed by the source code
+  /// rather than added later through mutation APIs or created with factory
----------------
Please clarify what happens to mixed content (some subtrees are from the source code, some are synthesized).


================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:90
+  /// functions.
+  bool isOriginal() const { return Original; }
+  /// If this function return false, the tree cannot be modified because there
----------------
Please also document what `isOriginal` means for syntax nodes that were moved within the tree (re-parented). For example, think of a refactoring that swaps "then" and "else" branches of an "if" statement.


================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:123
+  unsigned Original : 1;
+  unsigned CanModify : 1;
 };
----------------
Is it worth eagerly computing the "can modify" bit? Mapping expanded tokens to spelled is log(n) after all, and doing it for every syntax node can add up to nontrivial costs...


================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:163
+  /// Only used by MutationsImpl to implement higher-level mutation operations.
+  void replaceChildRangeLowLevel(Node *Before, Node *End, Node *New);
+  friend class MutationsImpl;
----------------
s/Before/BeforeBegin/? This way begin/end pairing is more obvious.


================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:483
+
+syntax::Leaf *syntax::Factory::createPunctuation(syntax::Arena &A,
+                                                 tok::TokenKind K) {
----------------
I can imagine that both building the syntax tree from the AST, and synthesizing syntax trees from thin air will eventually grow pretty long. So I'd like to suggest to split synthesis of syntax trees into a separate file, even though it will be pretty short today.


================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:498
+  return S;
+}
----------------
Please add the newline below.


================
Comment at: clang/lib/Tooling/Syntax/ComputeReplacements.cpp:20
+struct EnumerateTokenSpans {
+  EnumerateTokenSpans(const syntax::Tree *Root, ProcessTokensFn Callback)
+      : BeginSpan(nullptr), EndSpan(nullptr), SpanIsOriginal(false),
----------------
I'd probably change this struct into a function with a local lambda... I feel like it is weird to call a constructor just for the side-effects.


================
Comment at: clang/lib/Tooling/Syntax/ComputeReplacements.cpp:53
+  const syntax::Token *BeginSpan;
+  const syntax::Token *EndSpan;
+  bool SpanIsOriginal;
----------------
BeginOfSpan/EndOfSpan or SpanBegin/SpanEnd sound more natural to me.


================
Comment at: clang/lib/Tooling/Syntax/ComputeReplacements.cpp:79
+syntax::computeReplacements(const syntax::Arena &A,
+                            const syntax::TranslationUnit *TU) {
+  auto &Buffer = A.tokenBuffer();
----------------
Why not a reference for the TU?


================
Comment at: clang/lib/Tooling/Syntax/ComputeReplacements.cpp:86
+  std::string Replacement;
+  auto emitReplacement = [&](llvm::ArrayRef<syntax::Token> RemovedRange) {
+    if (RemovedRange.empty() && Replacement.empty())
----------------
s/RemovedRange/ReplacedRange/ ?


================
Comment at: clang/lib/Tooling/Syntax/ComputeReplacements.cpp:90
+
+    // Removed range points into expanded tokens.
+    assert(Buffer.expandedTokens().begin() <= RemovedRange.begin());
----------------
Unclear whether this comment is explaining the assumption, or explaining the assert.

"Check that the removed range points..."


================
Comment at: clang/lib/Tooling/Syntax/ComputeReplacements.cpp:92
+    assert(Buffer.expandedTokens().begin() <= RemovedRange.begin());
+    assert(RemovedRange.begin() < Buffer.expandedTokens().end());
+
----------------
This assertion duplicates the one in rangeOfExpanded; do we need to duplicate?

The code in this function does not depend on the assumption that tokens are expanded tokens; it is rangeOfExpanded that does.

If you decide to remove the assertion here, please move the comment from here into that function.


================
Comment at: clang/lib/Tooling/Syntax/ComputeReplacements.cpp:124
+  return Replacements;
+}
----------------
Please add the newline.


================
Comment at: clang/lib/Tooling/Syntax/Tree.cpp:89
+  else
+    FirstChild = New ? New : End;
+
----------------
Please mention in the doc comment that New can be null.


================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:57
+  return nullptr;
+}
+
----------------
Seems like these first/last helpers should be methods on `syntax::Node`.


================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:66
+  syntax::Leaf *Last = lastLeaf(T);
+  return llvm::makeArrayRef(First->token(), Last->token() + 1);
+}
----------------
The first and the last tokens are not necessarily from the same buffer...


================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:153
 
+  /// Finds the deepest nodes in the tree that covers exactly \p R.
+  /// FIXME: implement this efficiently and move to public syntax tree API.
----------------
"deepest node" (singular)


================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:523
+       )txt"},
+      // Some nodes are non-modifiable, they are marked with 'X:'.
+      {R"cpp(
----------------
I know this is 100% bikeshedding, but why "X"? I'd suggest "I" for "immutable". Feel free to ignore this comment though...


================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:528
+void test() {
+  HALF_IF HALF_IF_2 else {}
+})cpp",
----------------
Could you also do something like:

```
#define OPEN {
#define CLOSE }

void test1() {
  OPEN
    1;
  CLOSE
}
void test1() {
  OPEN
    1;
  }
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64573