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