r261297 - Implement the likely resolution of core issue 253.
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Fri Feb 19 17:01:29 PST 2016
On Fri, Feb 19, 2016 at 4:41 PM, Nico Weber <thakis at chromium.org> wrote:
> On Fri, Feb 19, 2016 at 4:29 PM, Richard Smith via cfe-commits
> <cfe-commits at lists.llvm.org> wrote:
>>
>> On Thu, Feb 18, 2016 at 5:52 PM, Nico Weber via cfe-commits
>> <cfe-commits at 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, 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.)
More information about the cfe-commits
mailing list