efriedma added a comment.
> restore E->getStorageDuration() == SD_Static check to fix libcxx regressions
Makes sense... but it would be nice to add a testcase for whatever construct
was triggering this.
================
Comment at: clang/lib/AST/Expr.cpp:3462-3468
->isConstantInitializer(Ctx, false, Culprit);
case CXXDefaultArgExprClass:
return cast<CXXDefaultArgExpr>(this)->getExpr()
->isConstantInitializer(Ctx, false, Culprit);
case CXXDefaultInitExprClass:
return cast<CXXDefaultInitExpr>(this)->getExpr()
->isConstantInitializer(Ctx, false, Culprit);
----------------
nickdesaulniers wrote:
> efriedma wrote:
> > nickdesaulniers wrote:
> > > @efriedma should a few more of these cases be changed from passing
> > > `false` as the `ForRef` param of `isConstantInitializer` to `IsForRef`?
> > > If I change the rest of the cases except for
> > > `MaterializeTemporaryExprClass` then tests are still green.
> > One of the reasons I didn't go with this approach is that I really didn't
> > want to think about this question...
> >
> > We probably don't have good test coverage for the C++ constructs, since we
> > don't really use isConstantInitializer() outside of C++03 mode.
> It looks like I should be able to test this against all of Android, and
> against Google's internal C++ codebase. Let me work through that, and see if
> I can shake out additional tests cases to add here so that we have lower risk
> of regression.
I think I'd prefer to go back to the original way I wrote this. I really just
don't want to think about the right way of translating the various new kinds of
lvalues that will be hitting this codepath.
I experimented with actually removing the relevant call to
isConstantInitializer (basically forcing C++03 code onto the C++11 codepath for
global vars), but it seemed sort of invasive/risky. Maybe still worth
considering at some point, I guess, but I don't want to mix too many things
into this patch.
My point with the test coverage was that this primarily impacts -std=c++03, in
a subtle way that won't be obvious in a lot of code. I'd be surprised if you
have significant amounts of code at Google that's still compiled with
-std=c++03.
================
Comment at: clang/lib/AST/Expr.cpp:3444
if (CE->getCastKind() == CK_NoOp ||
CE->getCastKind() == CK_LValueToRValue ||
CE->getCastKind() == CK_ToUnion ||
----------------
efriedma wrote:
> An CK_LValueToRValue conversion needs to change IsForRef (or else you're
> checking whether the address of the value is constant, not the value itself).
Not sure what I was thinking when I wrote the original comment here. We need
to bail on LValueToRValue here: the only way to correctly do an
lvalue-to-rvalue conversion is to evaluate the operand as an lvalue, then
convert the resulting lvalue to an rvalue. We need to use ExprConstant to do
that.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D151587/new/
https://reviews.llvm.org/D151587
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
