[PATCH] D81168: Add support for DeclRefExpr in SyntaxTree, by generating IdExpressions
Dmitri Gribenko via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Jun 8 07:05:27 PDT 2020
gribozavr2 added inline comments.
================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:190
+ return N->kind() == NodeKind::NameSpecifier;
+ }
+};
----------------
Should there be getters for various parts of the specifier?
================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:203
+
+/// An identifier expression, e.g. `n::S::a`. Modeled according to the grammar
+class IdExpression final : public Expression {
----------------
Could you add more details? Feel free to even quote the relevant grammar bits.
================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:211
+
+ syntax::Leaf *unqualifiedId();
+ syntax::NestedNameSpecifier *qualifier();
----------------
unqualified-id in the grammar can be more than one token.
================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:212
+ syntax::Leaf *unqualifiedId();
+ syntax::NestedNameSpecifier *qualifier();
+};
----------------
Could you swap the order of getters to match the source order?
================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:618
+ Builder.foldNode(Builder.getRange(it.getLocalSourceRange()), NS, nullptr);
+ Builder.markChild(NS, syntax::NodeRole::Unknown);
+ }
----------------
Do we need to mark the role if it is unknown?
================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:751
+void test(int b) {
+ ::a::b::S::f();
+}
----------------
Could you add more tests even if they fail today due to missing implementation?
`a::b::operator-(a::b::S())`
`a::b::S2<int>::f()`
etc. Basically cover the whole grammar in tests, if possible.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81168/new/
https://reviews.llvm.org/D81168
More information about the cfe-commits
mailing list