aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM
================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:1213
std::optional<PrimType> SubExprT = classify(SubExpr);
- if (E->getStorageDuration() == SD_Static) {
+ bool IsStatic = E->getStorageDuration() == SD_Static;
+ if (GlobalDecl || IsStatic) {
----------------
tbaeder wrote:
> aaron.ballman wrote:
> > tbaeder wrote:
> > > aaron.ballman wrote:
> > > > Should we be looking at the TLS kind of the extended declaration?
> > > > (`CheckCompleteVariableDeclaration()` is using `VarDecl::getTLSKind()
> > > > == VarDecl::TLS_Static`)
> > > >
> > > > Would something along these lines work instead?
> > > > ```
> > > > bool EmitGlobalTemp = E->getStorageDuration() == SD_Static;
> > > > if (!EmitGlobalTemp) {
> > > > if (const LifetimeExtendedTemporaryDecl *LETD =
> > > > E->getLifetimeExtendedTemporaryDecl()) {
> > > > if (const auto *VD =
> > > > dyn_cast_if_present<VarDecl>(LETD->getExtendingDecl()) {
> > > > EmitGlobalTemp= VD->getTLSKind() == VarDecl::TLS_Static;
> > > > }
> > > > }
> > > > }
> > > > ```
> > > That code definitely works for the current `AST/Interp/` tests, but we
> > > don't have tests for thread local stuff in there right now.
> > Hmm, I think we'll need those tests: https://eel.is/c++draft/expr.const#5.2
> >
> > That seems to be the only mention about thread local storage duration for
> > constant expressions in C++, so it might make sense to tackle that as part
> > of this change?
> >
> > (I worry that we'll forget to come back to this detail later, basically. So
> > either we should have failing test coverage showing we need a fix, an issue
> > in GitHub so we know to come back to it, etc. or just do the work up front
> > given that it's closely related.)
> I've pushed
> https://github.com/llvm/llvm-project/commit/11f5e5eb90c883d4b9ddba318e8fc57914b22ef3
>
> As you can see, the problem also exists for static variables. I think this
> needs additional checks at interpretation time, or even new opcodes to check
> the declaration when the initializer is computed. So I'd rather have this in
> a separate followup patch.
Follow-up patch is fine, thank you for the additional test coverage for it!
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156453/new/
https://reviews.llvm.org/D156453
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
