[PATCH] D88403: Migrate Declarators to use the List API

[Eduardo Caldas via Phabricator via cfe-commits]

eduucaldas added a reviewer: gribozavr2.
eduucaldas added inline comments.


================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:594
+    /// Shrink \p Range to a subrange that only contains tokens of a list.
+    ArrayRef<syntax::Token> shrinkToFitList(ArrayRef<syntax::Token> Range) {
+      auto BeginChildren = Trees.lower_bound(Range.begin());
----------------
WDYT about the naming?


================
Comment at: clang/unittests/Tooling/Syntax/BuildTreeTest.cpp:2932
   |-'void'
-  |-SimpleDeclarator Declarator
-  | |-'foo'
-  | `-ParametersAndQualifiers
-  |   |-'(' OpenParen
-  |   `-')' CloseParen
+  |-DeclaratorList Declarators
+  | `-SimpleDeclarator ListElement
----------------
This sounds weird and is probably the reason why SO many tests failed.

According to the [[ https://eel.is/c++draft/dcl.fct.def.general#nt:function-definition | grammar ]] a function definition/declaration might only have one declarator:
```
function-definition:
attribute-specifier-seq_opt decl-specifier-seq_opt declarator virt-specifier-seq_opt function-body
...
```
But our implementation calls `processDeclaratorAndDeclaration` for `DeclaratorDecl`, which englobes `FunctionDecl`.

I also noticed that we seem to have quite extensive support for declarations even rarer ones:
`StaticAssertDeclaration`, `LinkageSpecificationDeclaration` ... Moreover, their names and definitions seem to adequately follow the grammar. However we don't have a `FunctionDefinition` as defined in the grammar. Was grouping `FunctionDefintion` and `FunctionDeclaration` as `SimpleDeclaration`  a conscious choice? (I looked quickly at commits that added `processDeclaratorAndDeclaration` but couldn't find any clue)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88403