On Fri, Feb 19, 2016 at 10:32 PM, Nico Weber <tha...@chromium.org> wrote:
> 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.)
>
I think I like the approach of viewing this as a bug of isDefaultCtor().
Here's a patch that adds a few test cases and that also handles multiple
parameter packs followed by default arguments correctly. Please take a look.
>
>
>>
>>
>>> 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.)
>>>
>>
>>
>
Index: lib/AST/DeclCXX.cpp
===================================================================
--- lib/AST/DeclCXX.cpp (revision 261301)
+++ lib/AST/DeclCXX.cpp (working copy)
@@ -1792,8 +1792,12 @@
// C++ [class.ctor]p5:
// 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());
+ unsigned FirstNonPack = 0;
+ while (FirstNonPack < getNumParams() &&
+ getParamDecl(FirstNonPack)->isParameterPack())
+ ++FirstNonPack;
+ return FirstNonPack == getNumParams() ||
+ getParamDecl(FirstNonPack)->hasDefaultArg();
}
bool
Index: test/SemaCXX/cxx0x-cursory-default-delete.cpp
===================================================================
--- test/SemaCXX/cxx0x-cursory-default-delete.cpp (revision 261298)
+++ test/SemaCXX/cxx0x-cursory-default-delete.cpp (working copy)
@@ -74,6 +74,19 @@
struct no_fields_container {
no_fields nf;
};
+struct param_pack_ctor {
+ template <typename... T>
+ param_pack_ctor(T...);
+ int n;
+};
+struct param_pack_ctor_field {
+ param_pack_ctor ndc;
+};
+struct multi_param_pack_ctor {
+ template <typename... T, typename... U>
+ multi_param_pack_ctor(T..., U..., int f = 0);
+ int n;
+};
void constobjs() {
const no_fields nf; // ok
@@ -88,6 +101,9 @@
const some_init_container sicon; // expected-error {{default initialization
of an object of const type 'const some_init_container' without a user-provided
default constructor}}
const some_init_container_ctor siconc; // ok
const no_fields_container nfc; // ok
+ const param_pack_ctor ppc; // ok
+ const param_pack_ctor_field ppcf; // ok
+ const multi_param_pack_ctor mppc; // ok
}
struct non_const_derived : non_const_copy {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
