r261297 - Implement the likely resolution of core issue 253.
Nico Weber via cfe-commits
cfe-commits at lists.llvm.org
Fri Feb 19 19:32:08 PST 2016
On Fri, Feb 19, 2016 at 10:02 PM, Nico Weber <thakis at chromium.org> wrote:
> On Fri, Feb 19, 2016 at 8:01 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
>
>> 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,
>
>
> 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.)
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160219/7b3527d5/attachment-0001.html>
More information about the cfe-commits
mailing list