[PATCH] D88403: Migrate Declarators to use the List API
Dmitri Gribenko via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Sep 28 12:50:55 PDT 2020
gribozavr2 added inline comments.
================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:593
+ /// Shrink \p Range to a subrange that only contains tokens of a list.
+ ArrayRef<syntax::Token> shrinkToFitList(ArrayRef<syntax::Token> Range) {
----------------
================
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());
----------------
eduucaldas wrote:
> WDYT about the naming?
Very nice name!
================
Comment at: clang/unittests/Tooling/Syntax/BuildTreeTest.cpp:2932
|-'void'
- |-SimpleDeclarator Declarator
- | |-'foo'
- | `-ParametersAndQualifiers
- | |-'(' OpenParen
- | `-')' CloseParen
+ |-DeclaratorList Declarators
+ | `-SimpleDeclarator ListElement
----------------
eduucaldas wrote:
> 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)
It seemed reasonable for uniformity. However, I can definitely see an argument for defining a special kind of tree node for them, especially now that if we use SimpleDeclaration for functions we would always get a 1-element list wrapper.
On the other hand, the following is a valid declaration:
```
int x, f(int);
```
Wouldn't it be weird if function definitions and declarations were modeled differently? I understand they are different in the C++ grammar, but I think it would be at least a bit surprising for users.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88403/new/
https://reviews.llvm.org/D88403
More information about the cfe-commits
mailing list