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
