<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Feb 19, 2016 at 8:01 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"><div class=""><div class="h5">On Fri, Feb 19, 2016 at 4:41 PM, Nico Weber <<a href="mailto:thakis@chromium.org">thakis@chromium.org</a>> 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">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">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 constructor, even<br>
>> > for<br>
>> > classes without any fields. DR 253 relaxes this to say "If the 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 rule,<br>
>> > and e.g.<br>
>> > libstdc++ relies on something like DR 253 to compile in newer versions.<br>
>> > This<br>
>> > change makes it possible to build code that says `const 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>
>> > 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>
>> > <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>
>> > --- cfe/trunk/include/clang/AST/DeclCXX.h (original)<br>
>> > +++ cfe/trunk/include/clang/AST/DeclCXX.h Thu Feb 18 19:52:46 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 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 corresponding<br>
>> > special<br>
>> > /// member can't be fully analyzed without performing 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 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 constructor, we<br>
>> instead care whether the constructor that would have been chosen for<br>
>> initialization is defaulted (or equivalently, whether there *is* a<br>
>> defaulted default constructor, since if there is one, then either 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++ headers in<br>
> 5.x.<br>
> CXXConstructorDecl *CtorDecl = cast<CXXConstructorDecl>(Best->Function);<br>
> if (Kind.getKind() == InitializationKind::IK_Default &&<br>
> - Entity.getType().isConstQualified()) {<br>
> + Entity.getType().isConstQualified() && !CtorDecl->isUserProvided()) {<br>
> if (!CtorDecl->getParent()->allowConstDefaultInit()) {<br>
> if (!maybeRecoverWithZeroInitialization(S, Sequence, Entity))<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 should<br>
> be ok after this change. I think what you're suggesting is to 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 this 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 = S.LookupDefaultConstructor(ClassDecl);<br>
> + return !CD->isDefaulted();<br>
> }<br>
><br>
> /// \brief Determine whether this class has a destructor which has no<br>
><br>
> Now AST can't access Sema of course, so one way to do this would be to look<br>
> up the default ctor for every record in sema when completeDefinition() 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 current<br>
> computation, so maybe it makes sense to go back to lazy computation of this<br>
> information? Do you have any preferences for how to implement this?<br>
<br>
</div></div>We don't need to actually do overload resolution here. There are three 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 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 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,</blockquote><div><br></div><div>How would you know that in your example though? Actually, after reading the code a bit more, how about this instead:</div><div><br></div><div><div>Index: lib/AST/DeclCXX.cpp</div><div>===================================================================</div><div>--- lib/AST/DeclCXX.cpp<span class="" style="white-space:pre"> </span>(revision 261301)</div><div>+++ lib/AST/DeclCXX.cpp<span class="" style="white-space:pre"> </span>(working copy)</div></div><div><div>@@ -1793,7 +1803,8 @@</div><div> // A default constructor for a class X is a constructor of class</div><div> // X that can be called without an argument.</div><div> return (getNumParams() == 0) ||</div><div>- (getNumParams() > 0 && getParamDecl(0)->hasDefaultArg());</div><div>+ (getNumParams() > 0 && getParamDecl(0)->hasDefaultArg()) ||</div><div>+ (getNumParams() == 1 && getParamDecl(0)->isParameterPack());</div><div> }</div><div> </div><div> bool</div></div><div><br></div><div>Fixes the test cases, passes the test suite, and seems like a good change to me. For example, in</div><div><br></div><div><div>#include <type_traits></div><div>struct X {</div><div> template <typename... T,</div><div> typename = typename std::enable_if<sizeof...(T) != 0>::type></div><div> X(T...);</div><div> int n;</div><div>};</div><div>struct Y { X x; };</div><div>const Y y;</div></div><div><br></div><div>the explicit parameter pack deletes the implicit default ctor even though it's SFINAE'd out. (I tried to find a different example where this change makes an observable difference but so far I've failed to find one. This probably impacts other things though.)</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"> or one that was defaulted on its first declaration. We<br>
can either incrementally compute that in CXXRecordDecl, or move the<br>
caching to Sema and actually do the lookup. (The latter seems like it<br>
should generally not be a big deal, as we're doing more costly things<br>
to check the initialization anyway, and Sema caches special member<br>
lookups.)<br>
</blockquote></div><br></div></div>