[PATCH] D84348: WIP: Add complete id-expression support to syntax trees

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added inline comments.


================
Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:295
   syntax::NestedNameSpecifier *qualifier();
-  // TODO after expose `id-expression` from `DependentScopeDeclRefExpr`:
   // Add accessor for `template_opt`.
+  syntax::Leaf *templateKeyword();
----------------
You can remove this todo now.


================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:748
+  bool isDecltype(const Type &T) {
+    return T.getTypeClass() == Type::TypeClass::Decltype;
+  }
----------------
Please use `isa<DecltypeType>(T)` and inline this expression into its only user.


================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:753
+    return T.getTypeClass() == Type::TypeClass::TemplateSpecialization ||
+           T.getTypeClass() == Type::TypeClass::DependentTemplateSpecialization;
+  }
----------------
Ditto, please use `isa` and inline into the only user.



================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:803
+        // The 'template' keyword is always present in dependent template
+        // specializations
+        SR.setBegin(DependentTL.getTemplateKeywordLoc());
----------------
... except in the case of incorrect code. Feel free to just add a TODO.


================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:819
+      auto *NS = BuildNameSpecifier(*NNS);
+      if (!syntax::GlobalNameSpecifier::classof(NS))
+        Builder.foldNode(Builder.getRange(getLocalSourceRange(it)), NS,
----------------
Please use `isa<GlobalNameSpecifier>(NS)`.


================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:821
+        Builder.foldNode(Builder.getRange(getLocalSourceRange(it)), NS,
+                         nullptr);
+      Builder.markChild(NS, syntax::NodeRole::NestedNameSpecifier_specifier);
----------------
Could you add an overload for `foldNode` that takes a `NestedNameSpecifierLoc` but ignores it, just like we have an overload of `foldNode` that takes a `TypeLoc` but ignores it?


================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:834
+  // FIXME: I feel like this could be upstreamed.
+  SourceRange getUnqualifiedIdSourceRange(DeclRefExpr *S) {
+    if (S->hasExplicitTemplateArgs())
----------------
WDYM by "upstream"?


================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:844
 
+    if (auto TKL = S->getTemplateKeywordLoc(); TKL.isValid())
+      Builder.markChildToken(TKL, syntax::NodeRole::TemplateKeyword);
----------------
Please don't use C++17, Clang uses C++14 now.


================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:873
     template<typename T>
-    static T f(){}
+    struct TS {
+      static void f();
----------------
TS => ST (struct template?)


================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:879
+template<typename T>
+struct TS {
+  struct S {
----------------
ST


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84348