erik.pilkington added inline comments.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13121-13122
+ if (VD->isNoDestroy(getASTContext()) &&
+ !(VD->getType()->isArrayType() && getLangOpts().Exceptions &&
+ VD->isStaticLocal()))
return;
----------------
rjmccall wrote:
> erik.pilkington wrote:
> > rjmccall wrote:
> > > erik.pilkington wrote:
> > > > rjmccall wrote:
> > > > > rsmith wrote:
> > > > > > Hmm, perhaps it would be cleaner if the destructor for array
> > > > > > elements were required by the initialization of the array, instead
> > > > > > of here. That's how this works for struct aggregate initialization
> > > > > > (see `InitListChecker::CheckStructUnionTypes`), and (indirectly)
> > > > > > how it works for initialization by constructor, and so on. And it
> > > > > > reflects the fact that it's the initialization process that needs
> > > > > > the array element destructor, not the destruction of the variable
> > > > > > (which never happens).
> > > > > >
> > > > > > For the case of aggregate initialization of an array, we appear to
> > > > > > fail to implement [dcl.init.aggr]/8:
> > > > > >
> > > > > > """
> > > > > > The destructor for each element of class type is potentially
> > > > > > invoked (11.3.6) from the context where the aggregate
> > > > > > initialization occurs. [Note: This provision ensures that
> > > > > > destructors can be called for fully-constructed subobjects in case
> > > > > > an exception is thrown (14.2). — end note]
> > > > > > """
> > > > > >
> > > > > > (But there doesn't appear to be any corresponding wording for
> > > > > > default / value initialization of arrays. See also the special case
> > > > > > at the end of `BuildCXXNewExpr`.)
> > > > > That makes sense to me. The default/value initialization case seems
> > > > > like an obvious oversight in the standard, but I think it might be a
> > > > > distinction without a difference: the special cases for
> > > > > base-or-member initializers and new-expressions might cover literally
> > > > > every situation where initialization doesn't come with an adjacent
> > > > > need for non-exceptional destruction.
> > > > Sure, done. Moving this to initialization time seems like a nice
> > > > cleanup.
> > > Hmm. What I just mentioned for the documentation is relevant here,
> > > right? We only need to check access to subobject destructors if
> > > exceptions are enabled, so this attempt to avoid duplicate diagnostics
> > > might leave us not flagging the use if exceptions are disabled.
> > >
> > > Well, maybe the check isn't actually restricted that way, hmm. Shouldn't
> > > it be?
> > I thought we tried in general to keep `-fno-exceptions` as similar to
> > normal C++ as possible, rather than invent new language rules for it (even
> > when they make sense).
> There's at least one other place where `-fno-exceptions` changes the language
> rules: we don't look for `operator delete` in a new-expression. It does look
> like that's all I was remembering, though, and that we don't generally apply
> that same principle to destructors, so feel free to ignore my comment.
>
> Still, could you make sure there's a test case that tests that we check
> access to the destructor of an array variable even when exceptions are
> disabled, since that's the interesting corner case created by this new
> condition?
Sure, thats included in test/SemaCXX/no_destroy.cpp. Pretty much every test is
running in -fno-exceptions mode, since thats the default in `-cc1` for some
reason. All the more reason to keep the two modes as similar as possible I
guess :)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61165/new/
https://reviews.llvm.org/D61165
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
