rjmccall added inline comments.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:3915
+Here, if the construction of `array[9]` fails with an exception, `array[0..8]`
+will be destroyed, so the element's destructor needs to be accessible.
}];
----------------
Probably worth adding somewhere in here that is only required if exceptions are
enabled, since disabling exceptions is a pretty common configuration.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13121-13122
+ if (VD->isNoDestroy(getASTContext()) &&
+ !(VD->getType()->isArrayType() && getLangOpts().Exceptions &&
+ VD->isStaticLocal()))
return;
----------------
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?
================
Comment at: clang/lib/Sema/SemaInit.cpp:6328
+ if (hasAccessibleDestructor(S.Context.getBaseElementType(AT), Loc, S))
+ return ExprError();
+
----------------
There's no way that the right semantics are to return failure if the type has
an accessible destructor. :) Looks like the function is misnamed. Also, it
should also be named something that makes it clear that it's more than a
predicate, it's actually checking access to / marking the use of the
destructor. I know this is an existing function, but could you take care of
that?
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
