nickdesaulniers added inline comments.
================
Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1324
// This is a string literal initializing an array in an initializer.
- return CGM.GetConstantArrayFromStringLiteral(E);
+ return E->isLValue() ?
+ CGM.GetAddrOfConstantStringFromLiteral(E).getPointer() :
----------------
efriedma wrote:
> nickdesaulniers wrote:
> > nickdesaulniers wrote:
> > > efriedma wrote:
> > > > nickdesaulniers wrote:
> > > > > efriedma wrote:
> > > > > > Maybe we should have a separate ConstExprEmitterLValue... trying to
> > > > > > handle both LValues and RValues on the same codepath has been
> > > > > > problematic in the past. It's very easy for code to get confused
> > > > > > what it's actually trying to emit.
> > > > > So we'd have a `ConstExprEmitterLValue` class with some visitor
> > > > > methods, and a `ConstExprEmitterRValue` with other methods
> > > > > implemented?
> > > > Something like that.
> > > >
> > > > Actually thinking about it a bit more, not sure you need to actually
> > > > implement ConstExprEmitterLValue for now. You might just be able to
> > > > ensure we don't ever call ConstExprEmitter with an lvalue. The current
> > > > ConstExprEmitter doesn't expect lvalues, and shouldn't call itself with
> > > > lvalues. (We bail on explicit LValueToRValue conversions.) And
> > > > Evaluate() shouldn't actually evaluate the contents of an lvalue if it
> > > > isn't dereferenced, so there hopefully aren't any performance issues
> > > > using that codepath.
> > > >
> > > > In terms of implementation, I guess that's basically restoring the
> > > > destType->isReferenceType() that got removed? (I know I suggested it,
> > > > but I wasn't really thinking about it...)
> > > One thing I think we may need to add to `ConstExprEmitter` is the ability
> > > to evaluate `CallExpr`s based on certain test case failures...does that
> > > seem right?
> > See also the calls to `constexpr f()` in
> > clang/test/CodeGenCXX/const-init-cxx1y.cpp
> The only things I know of that Evaluate() can't handle are CK_ToUnion,
> CK_ReinterpretMemberPointer, and DesignatedInitUpdateExpr.
> DesignatedInitUpdateExpr is related to the failures in
> test/CodeGenCXX/designated-init.cpp; I don't think the others show up in any
> of the testcases you've mentioned. (CK_ToUnion can't appear in C++ code.
> CK_ReinterpretMemberPointer is a `reinterpret_cast<T>` where T is a member
> pointer type.)
>
> Given none of those constructs show up in const-init-cxx1y.cpp, the only
> reason for it to fail is if we aren't correctly falling back for a construct
> the current code doesn't know how to handle. You shouldn't need to implement
> any new constructs.
in clang/test/CodeGenCXX/designated-init.cpp we have:
```
>> 22 namespace ModifyStaticTemporary {
>>
23 struct A { int &&temporary; int x; };
24 constexpr int f(int &r) { r *= 9; return r - 12; }
25 A a = { 6, f(a.temporary) };
```
In the AST, that looks like:
```
| |-VarDecl 0x562b77df39b0 <line:25:3, col:29> col:5 used a
'A':'ModifyStaticTemporary::A' cinit
| | `-ExprWithCleanups 0x562b77df3c68 <col:9, col:29>
'A':'ModifyStaticTemporary::A'
| | `-InitListExpr 0x562b77df3bb8 <col:9, col:29>
'A':'ModifyStaticTemporary::A'
| | |-MaterializeTemporaryExpr 0x562b77df3c08 <col:11> 'int' xvalue
extended by Var 0x562b77df39b0 'a' 'A':'ModifyStaticTemporary::A'
| | | `-IntegerLiteral 0x562b77df3a18 <col:11> 'int' 6
| | `-CallExpr 0x562b77df3b30 <col:14, col:27> 'int'
| | |-ImplicitCastExpr 0x562b77df3b18 <col:14> 'int (*)(int &)'
<FunctionToPointerDecay>
| | | `-DeclRefExpr 0x562b77df3ad0 <col:14> 'int (int &)' lvalue Function
0x562b77df37a0 'f' 'int (int &)'
| | `-MemberExpr 0x562b77df3aa0 <col:16, col:18> 'int' lvalue .temporary
0x562b77df35c0
| | `-DeclRefExpr 0x562b77df3a80 <col:16>
'A':'ModifyStaticTemporary::A' lvalue Var 0x562b77df39b0 'a'
'A':'ModifyStaticTemporary::A'
```
(So, indeed no `DesignatedInitUpdateExpr`) but the call to the `constexpr`
`f()` updates the reference (to `54`). If I remove the visitor for
`MaterializeTemporaryExpr`, we fail to evaluate `f` and end up emitting `6`
rather than `54`. Doesn't that mean that the fast path (`ConstExprEmitter`)
needs to be able to evaluate `CallExpr`?
Or should `VisitInitListExpr` bail if any of the inits
`isa<MaterializeTemporaryExpr>` (or perhaps `isa<CallExpr>`)?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D151587/new/
https://reviews.llvm.org/D151587
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
