ayzhao added inline comments.
================
Comment at: clang/lib/CodeGen/CGExprAgg.cpp:581
+ Expr *filler = nullptr;
+ if (auto *ILE = dyn_cast<InitListExpr>(ExprToVisit))
+ filler = ILE->getArrayFiller();
----------------
ilya-biryukov wrote:
> ilya-biryukov wrote:
> > 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?
> Another important thing to address from the dump: notice how braced
> initialization creates `CXXDefaultInitExpr` and `CXXParenListInitExpr` copies
> the default argument expression directly. It's really important to use the
> former form, here's the example that currently breaks:
>
>
> ```
> #include <iostream>
>
> struct func_init { int some_int; const char* fn = __builtin_FUNCTION(); };
>
> int main() {
> func_init a(10);
> std::cout << "From paren init:" << a.fn << std::endl;
>
> func_init b{10};
> std::cout << "From braced init: " << b.fn << std::endl;
> }
> ```
>
> The program above is expected to report `main` for both cases, but instead we
> get:
> ```
> From paren init:
> From braced init: main
> ```
The following have now been implemented:
* `CXXDefaultInitExpr` and `ImplicitValueInitExpr`, which includes adding a
test for `__builtin_FUNCTION()`
* Array fillers
* Semantic forms vs syntactic forms
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
