<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Feb 24, 2016 at 3:53 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>On Wed, Feb 24, 2016 at 10:47 AM, Nico Weber <<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>> wrote:<br>
> Thanks for patiently explaining this. The attached patch is your email in<br>
> diff form. Does this look alright?<br>
<br>
</span>Yes, it looks great. Thanks for the excellent test cases.<br></blockquote><div><br></div><div>r261770, thanks!<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span><br>
> Since you mention C++98: We emit this diagnostic in C++98 mode (before and<br>
> after my change). The rule is new in C+++11, right? Should I add a check for<br>
> CPlusPlus11 before emitting this diagnostic (in a separate change)?<br>
<br>
</span>The rule was present (although worded differently), in paragraph 9:<br>
"If no initializer is specified for an object, and the object is of<br>
(possibly cv-qualified) non-POD class type (or array thereof), the<br>
object shall be default-initialized; if the object is of<br>
const-qualified type, the underlying class type shall have a<br>
user-declared default constructor."<br>
<span><br>
> I first forgot to undo my isDefaultCtor() change, and all the tests pass<br>
> both with and without it. Can you think of a test case that fails with the<br>
> isDefaultCtor() patch? (The new tests fail with _just_ the isDefaultCtor()<br>
> change.)<br>
<br>
</span>Looking through the uses, there seem to be roughly four different<br>
things going on:<br>
<br>
1) Checks for a trivial default constructor. These are unaffected by<br>
your change because a templated default constructor can never be<br>
trivial.<br>
2) Checks for a default constructor for diagnostic purposes. I think<br>
they'd be surprised if we called a constructor template a default<br>
constructor, especially if it's being passed arguments (the same is<br>
true in the default argument case, though).<br>
3) Checks for a default constructor for type traits and the like.<br>
These aren't really all that meaningful, but we probably shouldn't<br>
change their outcomes (we're emlating GCC behavior).<br>
4) A very small number of language semantic checks; the ones whose<br>
outcomes change seem to mostly be wrong (they want to find the<br>
constructor that overload resolution would actually select for a<br>
0-argument call).<br>
<br>
The easiest way to test this would probably be to static_assert that a<br>
class with a constructor template returns false for __is_trivial:<br>
<br>
struct X { template<typename ...T> X(T...); };<br>
static_assert(!__is_trivial(X), "");<br></blockquote><div><br></div><div>I had tried something like that (I think it was the `static_assert(__has_nothrow_constructor(X), "");` upthread), but I can't manage to behave it differently by changing isDefaultConstructor(). A mystery for another day, I suppose :-) Thanks again!</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div><br>
> On Tue, Feb 23, 2016 at 2:41 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>><br>
> wrote:<br>
>><br>
>> On Sat, Feb 20, 2016 at 6:53 AM, Nico Weber <<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>> wrote:<br>
>> > On Fri, Feb 19, 2016 at 10:32 PM, Nico Weber <<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>><br>
>> > wrote:<br>
>> >><br>
>> >> On Fri, Feb 19, 2016 at 10:02 PM, Nico Weber <<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>><br>
>> >> wrote:<br>
>> >>><br>
>> >>> On Fri, Feb 19, 2016 at 8:01 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>><br>
>> >>> wrote:<br>
>> >>>><br>
>> >>>> On Fri, Feb 19, 2016 at 4:41 PM, Nico Weber <<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>><br>
>> >>>> wrote:<br>
>> >>>> > On Fri, Feb 19, 2016 at 4:29 PM, Richard Smith via cfe-commits<br>
>> >>>> > <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
>> >>>> >><br>
>> >>>> >> On Thu, Feb 18, 2016 at 5:52 PM, Nico Weber via cfe-commits<br>
>> >>>> >> <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
>> >>>> >> > Author: nico<br>
>> >>>> >> > Date: Thu Feb 18 19:52:46 2016<br>
>> >>>> >> > New Revision: 261297<br>
>> >>>> >> ><br>
>> >>>> >> > URL: <a href="http://llvm.org/viewvc/llvm-project?rev=261297&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=261297&view=rev</a><br>
>> >>>> >> > Log:<br>
>> >>>> >> > Implement the likely resolution of core issue 253.<br>
>> >>>> >> ><br>
>> >>>> >> > C++11 requires const objects to have a user-provided<br>
>> >>>> >> > constructor,<br>
>> >>>> >> > even<br>
>> >>>> >> > for<br>
>> >>>> >> > classes without any fields. DR 253 relaxes this to say "If the<br>
>> >>>> >> > implicit<br>
>> >>>> >> > default<br>
>> >>>> >> > constructor initializes all subobjects, no initializer should be<br>
>> >>>> >> > required."<br>
>> >>>> >> ><br>
>> >>>> >> > clang is currently the only compiler that implements this C++11<br>
>> >>>> >> > rule,<br>
>> >>>> >> > and e.g.<br>
>> >>>> >> > libstdc++ relies on something like DR 253 to compile in newer<br>
>> >>>> >> > versions.<br>
>> >>>> >> > This<br>
>> >>>> >> > change makes it possible to build code that says `const<br>
>> >>>> >> > vector<int> v;'<br>
>> >>>> >> > again<br>
>> >>>> >> > when using libstdc++5.2 and _GLIBCXX_DEBUG<br>
>> >>>> >> > (<a href="https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60284" rel="noreferrer" target="_blank">https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60284</a>).<br>
>> >>>> >> ><br>
>> >>>> >> > Fixes PR23381.<br>
>> >>>> >> ><br>
>> >>>> >> > <a href="http://reviews.llvm.org/D16552" rel="noreferrer" target="_blank">http://reviews.llvm.org/D16552</a><br>
>> >>>> >> ><br>
>> >>>> >> > Modified:<br>
>> >>>> >> > cfe/trunk/include/clang/AST/DeclCXX.h<br>
>> >>>> >> > cfe/trunk/lib/AST/ASTImporter.cpp<br>
>> >>>> >> > cfe/trunk/lib/AST/DeclCXX.cpp<br>
>> >>>> >> > cfe/trunk/lib/Sema/SemaInit.cpp<br>
>> >>>> >> > cfe/trunk/lib/Serialization/ASTReaderDecl.cpp<br>
>> >>>> >> > cfe/trunk/lib/Serialization/ASTWriter.cpp<br>
>> >>>> >> ><br>
>> >>>> >> ><br>
>> >>>> >> > cfe/trunk/test/CXX/dcl.decl/dcl.fct.def/dcl.fct.def.default/p2.cpp<br>
>> >>>> >> > cfe/trunk/test/CXX/dcl.decl/dcl.init/p6.cpp<br>
>> >>>> >> > cfe/trunk/test/CXX/drs/dr4xx.cpp<br>
>> >>>> >> > cfe/trunk/test/SemaCXX/attr-selectany.cpp<br>
>> >>>> >> > cfe/trunk/test/SemaCXX/constexpr-value-init.cpp<br>
>> >>>> >> > cfe/trunk/test/SemaCXX/cxx0x-cursory-default-delete.cpp<br>
>> >>>> >> > cfe/trunk/test/SemaCXX/illegal-member-initialization.cpp<br>
>> >>>> >> > cfe/trunk/www/cxx_dr_status.html<br>
>> >>>> >> ><br>
>> >>>> >> > Modified: cfe/trunk/include/clang/AST/DeclCXX.h<br>
>> >>>> >> > URL:<br>
>> >>>> >> ><br>
>> >>>> >> ><br>
>> >>>> >> > <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclCXX.h?rev=261297&r1=261296&r2=261297&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclCXX.h?rev=261297&r1=261296&r2=261297&view=diff</a><br>
>> >>>> >> ><br>
>> >>>> >> ><br>
>> >>>> >> ><br>
>> >>>> >> > ==============================================================================<br>
>> >>>> >> > --- cfe/trunk/include/clang/AST/DeclCXX.h (original)<br>
>> >>>> >> > +++ cfe/trunk/include/clang/AST/DeclCXX.h Thu Feb 18 19:52:46<br>
>> >>>> >> > 2016<br>
>> >>>> >> > @@ -378,6 +378,10 @@ class CXXRecordDecl : public RecordDecl<br>
>> >>>> >> > /// even if the class has a trivial default constructor.<br>
>> >>>> >> > bool HasUninitializedReferenceMember : 1;<br>
>> >>>> >> ><br>
>> >>>> >> > + /// \brief True if any non-mutable field whose type doesn't<br>
>> >>>> >> > have a<br>
>> >>>> >> > user-<br>
>> >>>> >> > + /// provided default ctor also doesn't have an in-class<br>
>> >>>> >> > initializer.<br>
>> >>>> >> > + bool HasUninitializedFields : 1;<br>
>> >>>> >> > +<br>
>> >>>> >> > /// \brief These flags are \c true if a defaulted<br>
>> >>>> >> > corresponding<br>
>> >>>> >> > special<br>
>> >>>> >> > /// member can't be fully analyzed without performing<br>
>> >>>> >> > overload<br>
>> >>>> >> > resolution.<br>
>> >>>> >> > /// @{<br>
>> >>>> >> > @@ -1270,6 +1274,13 @@ public:<br>
>> >>>> >> > return !(data().HasTrivialSpecialMembers & SMF_Destructor);<br>
>> >>>> >> > }<br>
>> >>>> >> ><br>
>> >>>> >> > + /// \brief Determine whether declaring a const variable with<br>
>> >>>> >> > this<br>
>> >>>> >> > type is ok<br>
>> >>>> >> > + /// per core issue 253.<br>
>> >>>> >> > + bool allowConstDefaultInit() const {<br>
>> >>>> >> > + return !data().HasUninitializedFields ||<br>
>> >>>> >> > + hasUserProvidedDefaultConstructor();<br>
>> >>>> >><br>
>> >>>> >> hasUserProvidedDefaultConstructor() here is subtly incorrect. We<br>
>> >>>> >> shouldn't care whether there's a user-provided default<br>
>> >>>> >> constructor,<br>
>> >>>> >> we<br>
>> >>>> >> instead care whether the constructor that would have been chosen<br>
>> >>>> >> for<br>
>> >>>> >> initialization is defaulted (or equivalently, whether there *is* a<br>
>> >>>> >> defaulted default constructor, since if there is one, then either<br>
>> >>>> >> the<br>
>> >>>> >> initialization is ambiguous or it is chosen).<br>
>> >>>> >><br>
>> >>>> >> This causes a regression for a testcase such as:<br>
>> >>>> >><br>
>> >>>> >> struct X { template<typename ...T> X(T...); int n; };<br>
>> >>>> >> const X x; // formerly OK, now bogus error<br>
>> >>>> ><br>
>> >>>> ><br>
>> >>>> > Hm, great point. For a single hop, this fixes it:<br>
>> >>>> ><br>
>> >>>> > Index: lib/Sema/SemaInit.cpp<br>
>> >>>> > ===================================================================<br>
>> >>>> > --- lib/Sema/SemaInit.cpp (revision 261298)<br>
>> >>>> > +++ lib/Sema/SemaInit.cpp (working copy)<br>
>> >>>> > @@ -3521,7 +3521,7 @@<br>
>> >>>> > // The 253 proposal is for example needed to process libstdc++<br>
>> >>>> > headers in<br>
>> >>>> > 5.x.<br>
>> >>>> > CXXConstructorDecl *CtorDecl =<br>
>> >>>> > cast<CXXConstructorDecl>(Best->Function);<br>
>> >>>> > if (Kind.getKind() == InitializationKind::IK_Default &&<br>
>> >>>> > - Entity.getType().isConstQualified()) {<br>
>> >>>> > + Entity.getType().isConstQualified() &&<br>
>> >>>> > !CtorDecl->isUserProvided()) {<br>
>> >>>> > if (!CtorDecl->getParent()->allowConstDefaultInit()) {<br>
>> >>>> > if (!maybeRecoverWithZeroInitialization(S, Sequence,<br>
>> >>>> > Entity))<br>
>> >>>> ><br>
>> >>>> > Sequence.SetFailed(InitializationSequence::FK_DefaultInitOfConst);<br>
>> >>>> ><br>
>> >>>> ><br>
>> >>>> > However, it doesn't make this pass:<br>
>> >>>> ><br>
>> >>>> > struct X { template<typename ...T> X(T...); int n; };<br>
>> >>>> > struct Y { X x; };<br>
>> >>>> > const Y y;<br>
>> >>>> ><br>
>> >>>> > That didn't build before this change either, but it feels like this<br>
>> >>>> > should<br>
>> >>>> > be ok after this change. I think what you're suggesting is to<br>
>> >>>> > conceptually<br>
>> >>>> > do this instead:<br>
>> >>>> ><br>
>> >>>> > Index: include/clang/AST/DeclCXX.h<br>
>> >>>> > ===================================================================<br>
>> >>>> > --- include/clang/AST/DeclCXX.h (revision 261298)<br>
>> >>>> > +++ include/clang/AST/DeclCXX.h (working copy)<br>
>> >>>> > @@ -1277,8 +1277,10 @@<br>
>> >>>> > /// \brief Determine whether declaring a const variable with<br>
>> >>>> > this<br>
>> >>>> > type is<br>
>> >>>> > ok<br>
>> >>>> > /// per core issue 253.<br>
>> >>>> > bool allowConstDefaultInit() const {<br>
>> >>>> > - return !data().HasUninitializedFields ||<br>
>> >>>> > - hasUserProvidedDefaultConstructor();<br>
>> >>>> > + if (!data().HasUninitializedFields)<br>
>> >>>> > + return true;<br>
>> >>>> > + CXXConstructorDecl *CD =<br>
>> >>>> > S.LookupDefaultConstructor(ClassDecl);<br>
>> >>>> > + return !CD->isDefaulted();<br>
>> >>>> > }<br>
>> >>>> ><br>
>> >>>> > /// \brief Determine whether this class has a destructor which<br>
>> >>>> > has<br>
>> >>>> > no<br>
>> >>>> ><br>
>> >>>> > Now AST can't access Sema of course, so one way to do this would be<br>
>> >>>> > to<br>
>> >>>> > look<br>
>> >>>> > up the default ctor for every record in sema when<br>
>> >>>> > completeDefinition()<br>
>> >>>> > for a<br>
>> >>>> > record is called and then do<br>
>> >>>> ><br>
>> >>>> > if (!CD->isDefaulted())<br>
>> >>>> > RD->setAllowConstDefaultInit(true);<br>
>> >>>> ><br>
>> >>>> > But looking up the constructor is a bit more expensive than the<br>
>> >>>> > current<br>
>> >>>> > computation, so maybe it makes sense to go back to lazy computation<br>
>> >>>> > of<br>
>> >>>> > this<br>
>> >>>> > information? Do you have any preferences for how to implement this?<br>
>> >>>><br>
>> >>>> We don't need to actually do overload resolution here. There are<br>
>> >>>> three<br>
>> >>>> cases:<br>
>> >>>><br>
>> >>>> 0) Default initialization is ill-formed in some way => we don't care<br>
>> >>>> what this function returns<br>
>> >>>> 1) There is no defaulted default constructor => const default init is<br>
>> >>>> OK<br>
>> >>>> 2) There is a defaulted default constructor => default init must use<br>
>> >>>> it (any alternative would put us in case 0), const default init is<br>
>> >>>> not<br>
>> >>>> OK if we have uninitialized fields<br>
>> >>>><br>
>> >>>> So we only need to know if there is either an implicit default<br>
>> >>>> constructor,<br>
>> >>><br>
>> >>><br>
>> >>> How would you know that in your example though?<br>
>><br>
>> When a constructor is added to the class, we can check whether it's a<br>
>> defaulted default constructor (that is, add another flag to<br>
>> DefinitionData to track whether there is one)[1]. Then the class has a<br>
>> defaulted default constructor if either (a) you've seen one, or (b)<br>
>> the class needs one to be implicitly declared (we already have a flag<br>
>> for that on CXXRecordDecl).<br>
>><br>
>> So, we allow default const init if<br>
>> !(HasDeclaredDefaultedDefaultConstructor ||<br>
>> needsImplicitDefaultConstructor()) || !HasUninitializedFields.<br>
>><br>
>> [1]: I'm not sure whether we set an implicit default constructor to<br>
>> be defaulted in C++98 mode. If not, you can check whether it's<br>
>> defaulted or implicit.<br>
>><br>
>> >>> Actually, after reading<br>
>> >>> the code a bit more, how about this instead:<br>
>> >>><br>
>> >>> Index: lib/AST/DeclCXX.cpp<br>
>> >>> ===================================================================<br>
>> >>> --- lib/AST/DeclCXX.cpp (revision 261301)<br>
>> >>> +++ lib/AST/DeclCXX.cpp (working copy)<br>
>> >>> @@ -1793,7 +1803,8 @@<br>
>> >>> // A default constructor for a class X is a constructor of class<br>
>> >>> // X that can be called without an argument.<br>
>> >>> return (getNumParams() == 0) ||<br>
>> >>> - (getNumParams() > 0 && getParamDecl(0)->hasDefaultArg());<br>
>> >>> + (getNumParams() > 0 && getParamDecl(0)->hasDefaultArg()) ||<br>
>> >>> + (getNumParams() == 1 && getParamDecl(0)->isParameterPack());<br>
>> >>> }<br>
>> >>><br>
>> >>> bool<br>
>> >>><br>
>> >>> Fixes the test cases, passes the test suite, and seems like a good<br>
>> >>> change<br>
>> >>> to me. For example, in<br>
>> >>><br>
>> >>> #include <type_traits><br>
>> >>> struct X {<br>
>> >>> template <typename... T,<br>
>> >>> typename = typename std::enable_if<sizeof...(T) !=<br>
>> >>> 0>::type><br>
>> >>> X(T...);<br>
>> >>> int n;<br>
>> >>> };<br>
>> >>> struct Y { X x; };<br>
>> >>> const Y y;<br>
>> >>><br>
>> >>> the explicit parameter pack deletes the implicit default ctor even<br>
>> >>> though<br>
>> >>> it's SFINAE'd out. (I tried to find a different example where this<br>
>> >>> change<br>
>> >>> makes an observable difference but so far I've failed to find one.<br>
>> >>> This<br>
>> >>> probably impacts other things though.)<br>
>> >><br>
>> >><br>
>> >> After looking through the callers of isDefaultConstructor(), maybe it<br>
>> >> actually doesn't affect other things. It does make it possible to fix<br>
>> >> the<br>
>> >> following example with a another small tweak:<br>
>> >><br>
>> >> struct X { template<typename ...T> constexpr X(T...) noexcept {} };<br>
>> >> static_assert(__has_nothrow_constructor(X), "");<br>
>> >><br>
>> >> (clang currently rejects this, but gcc accepts it, and it looks like it<br>
>> >> ought to be accepted.)<br>
>> ><br>
>> ><br>
>> > I think I like the approach of viewing this as a bug of isDefaultCtor().<br>
>> > Here's a patch that adds a few test cases and that also handles multiple<br>
>> > parameter packs followed by default arguments correctly. Please take a<br>
>> > look.<br>
>><br>
>> That change is not correct :( Consider a class like this:<br>
>><br>
>> struct X {<br>
>> template<typename ...T, typename enable_if<sizeof...(T) != 0>::type*<br>
>> = nullptr> X(T...);<br>
>> X() = default;<br>
>> };<br>
>><br>
>> Here, the constructor template is not a default constructor. This<br>
>> class has only one default constructor, and it is defaulted.<br>
><br>
><br>
</div></div></blockquote></div><br></div></div>