ilya-biryukov added a comment.
Thanks for addressing the comments and sorry for a wait before the comments.
Please see the comment about syntactic form, I might be holding it wrong, but
it looks like we actually loose the syntactic form completely in this case.
================
Comment at: clang/lib/AST/JSONNodeDumper.cpp:852
case VarDecl::ListInit: JOS.attribute("init", "list"); break;
+ case VarDecl::ParenListInit:
+ JOS.attribute("init", "paren-list");
----------------
ayzhao wrote:
> ilya-biryukov wrote:
> > NIT: maybe use the same formatting as other switch cases for constistency?
> Unfortunately clang-format insists that these be on separate lines.
Ah, that's unfortunate. I normally just revert the effect of `clang-format` for
those lines, but up to you.
Also fine to keep as is or even format the other lines according to the style
guide (it's just 3 more lines, so should not be a big deal).
================
Comment at: clang/lib/CodeGen/CGExprAgg.cpp:581
+ Expr *filler = nullptr;
+ if (auto *ILE = dyn_cast<InitListExpr>(ExprToVisit))
+ filler = ILE->getArrayFiller();
----------------
ayzhao wrote:
> ilya-biryukov wrote:
> > - Should we have a filler for `CXXParenInitListExpr` too?
> > It seems like an important optimization and could have large effect on
> > compile times if we don't
> >
> > - Same question for semantic and syntactic-form (similar to
> > `InitListExpr`): should we have it here?
> > I am not sure if it's semantically required (probably not?), but that's
> > definitely something that `clang-tidy` and other source tools will rely on.
> >
> > We should probably share the code there, I suggest moving it to a shared
> > base class and using it where appropriate instead of the derived classes.
> > Should we have a filler for CXXParenInitListExpr too? It seems like an
> > important optimization and could have large effect on compile times if we
> > don't
>
> This feels like premature optimization - presumably, wouldn't this only be an
> issue with extraordinarily large (say, O(1000)) arrays?
>
> > Same question for semantic and syntactic-form (similar to InitListExpr):
> > should we have it here? I am not sure if it's semantically required
> > (probably not?), but that's definitely something that clang-tidy and other
> > source tools will rely on
>
> IIRC this doesn't apply to paren list aggregate expressions, as the syntatic
> form would be the enclosing `ParenListExpr`.
> This feels like premature optimization - presumably, wouldn't this only be an
> issue with extraordinarily large (say, O(1000)) arrays?
Yes, this should only happen with large arrays. Normally I would agree, but
it's surprising that changing `{}` to `()` in the compiler would lead to
performance degradation.
In that sense, this premature optimization is already implemented, we are
rather degrading performance for a different syntax to do the same thing.
Although we could also land without it, but in that case could you open a GH
issue and add a FIXME to track the implementation of this particular
optimization?
This should increase the chances of users finding the root cause of the issue
if they happen to hit it.
> IIRC this doesn't apply to paren list aggregate expressions, as the syntatic
> form would be the enclosing ParenListExpr.
Do we even have the enclosing `ParenListExpr`? If I dump the AST with `clang
-fsyntax-only -Xclang=-ast-dump ...` for the following code:
```
struct pair { int a; int b = 2; };
int main() {
pair(1); pair p(1);
pair b{1}; pair{1};
}
```
I get
```
`-FunctionDecl 0x557d79717e98 <line:2:1, line:5:1> line:2:5 main 'int ()'
`-CompoundStmt 0x557d797369d0 <col:12, line:5:1>
|-CXXFunctionalCastExpr 0x557d79718528 <line:3:3, col:9> 'pair':'pair'
functional cast to pair <NoOp>
| `-CXXParenListInitExpr 0x557d79718500 <col:3> 'pair':'pair'
| |-IntegerLiteral 0x557d79718010 <col:8> 'int' 1
| `-IntegerLiteral 0x557d79717e18 <line:1:30> 'int' 2
|-DeclStmt 0x557d79718650 <line:3:12, col:21>
| `-VarDecl 0x557d79718568 <col:12, col:17> col:17 p 'pair':'pair'
parenlistinit
| `-CXXParenListInitExpr 0x557d79718610 <col:17> 'pair':'pair'
| |-IntegerLiteral 0x557d797185d0 <col:19> 'int' 1
| `-IntegerLiteral 0x557d79717e18 <line:1:30> 'int' 2
|-DeclStmt 0x557d797187d8 <line:4:3, col:12>
| `-VarDecl 0x557d79718680 <col:3, col:11> col:8 b 'pair':'pair' listinit
| `-InitListExpr 0x557d79718750 <col:9, col:11> 'pair':'pair'
| |-IntegerLiteral 0x557d797186e8 <col:10> 'int' 1
| `-CXXDefaultInitExpr 0x557d797187a0 <col:11> 'int'
`-CXXFunctionalCastExpr 0x557d797369a8 <col:14, col:20> 'pair':'pair'
functional cast to pair <NoOp>
`-InitListExpr 0x557d79718868 <col:18, col:20> 'pair':'pair'
|-IntegerLiteral 0x557d79718800 <col:19> 'int' 1
`-CXXDefaultInitExpr 0x557d797188b8 <col:20> 'int'
```
It feels like the `ParentListExpr` is replaced during semantic analysis and
there is no way to get it back. I also tried running `clang-query` and trying
to `match parenListExpr()` and go 0 results. Is it just missing in the AST dump
and I run `clang-query` incorrectly or do we actually not have the syntactic
form of this expression after all?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129531/new/
https://reviews.llvm.org/D129531
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
