On Fri, Feb 19, 2016 at 10:02 PM, Nico Weber <tha...@chromium.org> wrote:
> On Fri, Feb 19, 2016 at 8:01 PM, Richard Smith <rich...@metafoo.co.uk> > wrote: > >> On Fri, Feb 19, 2016 at 4:41 PM, Nico Weber <tha...@chromium.org> wrote: >> > On Fri, Feb 19, 2016 at 4:29 PM, Richard Smith via cfe-commits >> > <cfe-commits@lists.llvm.org> wrote: >> >> >> >> On Thu, Feb 18, 2016 at 5:52 PM, Nico Weber via cfe-commits >> >> <cfe-commits@lists.llvm.org> wrote: >> >> > Author: nico >> >> > Date: Thu Feb 18 19:52:46 2016 >> >> > New Revision: 261297 >> >> > >> >> > URL: http://llvm.org/viewvc/llvm-project?rev=261297&view=rev >> >> > Log: >> >> > Implement the likely resolution of core issue 253. >> >> > >> >> > C++11 requires const objects to have a user-provided constructor, >> even >> >> > for >> >> > classes without any fields. DR 253 relaxes this to say "If the >> implicit >> >> > default >> >> > constructor initializes all subobjects, no initializer should be >> >> > required." >> >> > >> >> > clang is currently the only compiler that implements this C++11 rule, >> >> > and e.g. >> >> > libstdc++ relies on something like DR 253 to compile in newer >> versions. >> >> > This >> >> > change makes it possible to build code that says `const vector<int> >> v;' >> >> > again >> >> > when using libstdc++5.2 and _GLIBCXX_DEBUG >> >> > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60284). >> >> > >> >> > Fixes PR23381. >> >> > >> >> > http://reviews.llvm.org/D16552 >> >> > >> >> > Modified: >> >> > cfe/trunk/include/clang/AST/DeclCXX.h >> >> > cfe/trunk/lib/AST/ASTImporter.cpp >> >> > cfe/trunk/lib/AST/DeclCXX.cpp >> >> > cfe/trunk/lib/Sema/SemaInit.cpp >> >> > cfe/trunk/lib/Serialization/ASTReaderDecl.cpp >> >> > cfe/trunk/lib/Serialization/ASTWriter.cpp >> >> > >> cfe/trunk/test/CXX/dcl.decl/dcl.fct.def/dcl.fct.def.default/p2.cpp >> >> > cfe/trunk/test/CXX/dcl.decl/dcl.init/p6.cpp >> >> > cfe/trunk/test/CXX/drs/dr4xx.cpp >> >> > cfe/trunk/test/SemaCXX/attr-selectany.cpp >> >> > cfe/trunk/test/SemaCXX/constexpr-value-init.cpp >> >> > cfe/trunk/test/SemaCXX/cxx0x-cursory-default-delete.cpp >> >> > cfe/trunk/test/SemaCXX/illegal-member-initialization.cpp >> >> > cfe/trunk/www/cxx_dr_status.html >> >> > >> >> > Modified: cfe/trunk/include/clang/AST/DeclCXX.h >> >> > URL: >> >> > >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclCXX.h?rev=261297&r1=261296&r2=261297&view=diff >> >> > >> >> > >> ============================================================================== >> >> > --- cfe/trunk/include/clang/AST/DeclCXX.h (original) >> >> > +++ cfe/trunk/include/clang/AST/DeclCXX.h Thu Feb 18 19:52:46 2016 >> >> > @@ -378,6 +378,10 @@ class CXXRecordDecl : public RecordDecl >> >> > /// even if the class has a trivial default constructor. >> >> > bool HasUninitializedReferenceMember : 1; >> >> > >> >> > + /// \brief True if any non-mutable field whose type doesn't >> have a >> >> > user- >> >> > + /// provided default ctor also doesn't have an in-class >> >> > initializer. >> >> > + bool HasUninitializedFields : 1; >> >> > + >> >> > /// \brief These flags are \c true if a defaulted corresponding >> >> > special >> >> > /// member can't be fully analyzed without performing overload >> >> > resolution. >> >> > /// @{ >> >> > @@ -1270,6 +1274,13 @@ public: >> >> > return !(data().HasTrivialSpecialMembers & SMF_Destructor); >> >> > } >> >> > >> >> > + /// \brief Determine whether declaring a const variable with this >> >> > type is ok >> >> > + /// per core issue 253. >> >> > + bool allowConstDefaultInit() const { >> >> > + return !data().HasUninitializedFields || >> >> > + hasUserProvidedDefaultConstructor(); >> >> >> >> hasUserProvidedDefaultConstructor() here is subtly incorrect. We >> >> shouldn't care whether there's a user-provided default constructor, we >> >> instead care whether the constructor that would have been chosen for >> >> initialization is defaulted (or equivalently, whether there *is* a >> >> defaulted default constructor, since if there is one, then either the >> >> initialization is ambiguous or it is chosen). >> >> >> >> This causes a regression for a testcase such as: >> >> >> >> struct X { template<typename ...T> X(T...); int n; }; >> >> const X x; // formerly OK, now bogus error >> > >> > >> > Hm, great point. For a single hop, this fixes it: >> > >> > Index: lib/Sema/SemaInit.cpp >> > =================================================================== >> > --- lib/Sema/SemaInit.cpp (revision 261298) >> > +++ lib/Sema/SemaInit.cpp (working copy) >> > @@ -3521,7 +3521,7 @@ >> > // The 253 proposal is for example needed to process libstdc++ >> headers in >> > 5.x. >> > CXXConstructorDecl *CtorDecl = >> cast<CXXConstructorDecl>(Best->Function); >> > if (Kind.getKind() == InitializationKind::IK_Default && >> > - Entity.getType().isConstQualified()) { >> > + Entity.getType().isConstQualified() && >> !CtorDecl->isUserProvided()) { >> > if (!CtorDecl->getParent()->allowConstDefaultInit()) { >> > if (!maybeRecoverWithZeroInitialization(S, Sequence, Entity)) >> > >> Sequence.SetFailed(InitializationSequence::FK_DefaultInitOfConst); >> > >> > >> > However, it doesn't make this pass: >> > >> > struct X { template<typename ...T> X(T...); int n; }; >> > struct Y { X x; }; >> > const Y y; >> > >> > That didn't build before this change either, but it feels like this >> should >> > be ok after this change. I think what you're suggesting is to >> conceptually >> > do this instead: >> > >> > Index: include/clang/AST/DeclCXX.h >> > =================================================================== >> > --- include/clang/AST/DeclCXX.h (revision 261298) >> > +++ include/clang/AST/DeclCXX.h (working copy) >> > @@ -1277,8 +1277,10 @@ >> > /// \brief Determine whether declaring a const variable with this >> type is >> > ok >> > /// per core issue 253. >> > bool allowConstDefaultInit() const { >> > - return !data().HasUninitializedFields || >> > - hasUserProvidedDefaultConstructor(); >> > + if (!data().HasUninitializedFields) >> > + return true; >> > + CXXConstructorDecl *CD = S.LookupDefaultConstructor(ClassDecl); >> > + return !CD->isDefaulted(); >> > } >> > >> > /// \brief Determine whether this class has a destructor which has no >> > >> > Now AST can't access Sema of course, so one way to do this would be to >> look >> > up the default ctor for every record in sema when completeDefinition() >> for a >> > record is called and then do >> > >> > if (!CD->isDefaulted()) >> > RD->setAllowConstDefaultInit(true); >> > >> > But looking up the constructor is a bit more expensive than the current >> > computation, so maybe it makes sense to go back to lazy computation of >> this >> > information? Do you have any preferences for how to implement this? >> >> We don't need to actually do overload resolution here. There are three >> cases: >> >> 0) Default initialization is ill-formed in some way => we don't care >> what this function returns >> 1) There is no defaulted default constructor => const default init is OK >> 2) There is a defaulted default constructor => default init must use >> it (any alternative would put us in case 0), const default init is not >> OK if we have uninitialized fields >> >> So we only need to know if there is either an implicit default >> constructor, > > > How would you know that in your example though? Actually, after reading > the code a bit more, how about this instead: > > Index: lib/AST/DeclCXX.cpp > =================================================================== > --- lib/AST/DeclCXX.cpp (revision 261301) > +++ lib/AST/DeclCXX.cpp (working copy) > @@ -1793,7 +1803,8 @@ > // A default constructor for a class X is a constructor of class > // X that can be called without an argument. > return (getNumParams() == 0) || > - (getNumParams() > 0 && getParamDecl(0)->hasDefaultArg()); > + (getNumParams() > 0 && getParamDecl(0)->hasDefaultArg()) || > + (getNumParams() == 1 && getParamDecl(0)->isParameterPack()); > } > > bool > > Fixes the test cases, passes the test suite, and seems like a good change > to me. For example, in > > #include <type_traits> > struct X { > template <typename... T, > typename = typename std::enable_if<sizeof...(T) != 0>::type> > X(T...); > int n; > }; > struct Y { X x; }; > const Y y; > > 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.) > After looking through the callers of isDefaultConstructor(), maybe it actually doesn't affect other things. It does make it possible to fix the following example with a another small tweak: struct X { template<typename ...T> constexpr X(T...) noexcept {} }; static_assert(__has_nothrow_constructor(X), ""); (clang currently rejects this, but gcc accepts it, and it looks like it ought to be accepted.) > > >> or one that was defaulted on its first declaration. We >> can either incrementally compute that in CXXRecordDecl, or move the >> caching to Sema and actually do the lookup. (The latter seems like it >> should generally not be a big deal, as we're doing more costly things >> to check the initialization anyway, and Sema caches special member >> lookups.) >> > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
