erik.pilkington added inline comments.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:3897-3900
+This works in almost all cases, but if ``no_destroy`` is applied to a
``static``
+or ``thread_local`` local builtin array variable and exceptions are enabled,
the
+destructor is still needed to perform cleanup if the construction of an element
+of the array throws. For instance:
----------------
rsmith wrote:
> I think this is the wrong way to think about and describe this.
> `[[no_destroy]]` removes the need for the type of the variable to have a
> usable destructor. But all immediate subobjects still need usable
> destructors, in case an exception is thrown during the object's construction.
> This is then identical to the constraints on `new T` -- subobjects of `T`
> (including array elements) still need to be destructible, but `T` itself does
> not.
Sure, that makes sense. The new patch rephrases this.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13119
+ // variable is static local array and exceptions are enabled, since then we
+ // need to clean up the elements.
+ if (VD->isNoDestroy(getASTContext()) &&
----------------
rjmccall wrote:
> This isn't "emitting" the destructor, it's "using" it. Also, the comment
> should make it clear that this is about aggregate initialization in general,
> and it should contain a FIXME if there's part of that rule we're not
> implementing correctly.
Are you alluding to the CodeGen bug? That seems unrelated to this function.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13121-13122
+ if (VD->isNoDestroy(getASTContext()) &&
+ !(VD->getType()->isArrayType() && getLangOpts().Exceptions &&
+ VD->isStaticLocal()))
return;
----------------
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.
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
