erik.pilkington marked 2 inline comments as done.
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:
> > > 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).
================
Comment at: clang/lib/Sema/SemaInit.cpp:6328
+ if (hasAccessibleDestructor(S.Context.getBaseElementType(AT), Loc, S))
+ return ExprError();
+
----------------
rjmccall wrote:
> 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?
Yeah, the name doesn't make much sense. I'll update it...
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
