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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
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:
----------------
gribozavr2 wrote:
> 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?
Good point, this pattern is terrible.
It is for friendship, but this is supposed to be an implementation detail, therefore shouldn't affect user interface.

I turned these into free-standing functions. Not sure if we should group those into a namespace for simpler discoverability via code completion (although one can just do `syntax::create^` to get the same effect now) 


================
Comment at: clang/include/clang/Tooling/Syntax/Tree.h:123
+  unsigned Original : 1;
+  unsigned CanModify : 1;
 };
----------------
gribozavr2 wrote:
> 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...
I would expect this bit to be requested quite often, so I think this would actually pay off in the long run. Requesting this bit should be very cheap.

Note that we do quite a bit of `O(log N)` searches in the same code that sets `CanModify` bit, so it does not actually make the complexity worse, but definitely makes the constant larger.

If this starts being the bottleneck (I bet this will happen eventually), I believe we can compute it eagerly with `O(1)` amortized cost. That would involve traversing the AST in the **true** source code order, which will require tweaking the `RecursiveASTVisitor` and is much easier done when we have good test coverage with existing implementation.


================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:483
+
+syntax::Leaf *syntax::Factory::createPunctuation(syntax::Arena &A,
+                                                 tok::TokenKind K) {
----------------
gribozavr2 wrote:
> 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.
Good point.
I've moved only the implementation for now, declaration of synthesis functions are still in `BuildTree.h` as it seems small enough for now.

I can totally imagine us moving those to a separate file later, though.


================
Comment at: clang/lib/Tooling/Syntax/ComputeReplacements.cpp:79
+syntax::computeReplacements(const syntax::Arena &A,
+                            const syntax::TranslationUnit *TU) {
+  auto &Buffer = A.tokenBuffer();
----------------
gribozavr2 wrote:
> Why not a reference for the TU?
All accessors on syntax tree return pointers (almost everything can be null), so using pointers everywhere just makes the code more uniform.

This one does look a bit different, though, agree that using a reference in this public API is probably more natural.


================
Comment at: clang/lib/Tooling/Syntax/ComputeReplacements.cpp:92
+    assert(Buffer.expandedTokens().begin() <= RemovedRange.begin());
+    assert(RemovedRange.begin() < Buffer.expandedTokens().end());
+
----------------
gribozavr2 wrote:
> 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.
Good point, thanks. Moved the comment to `rangeOfExpanded`, we don't need to assert in both places.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64573