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.) > 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
