Thanks for patiently explaining this. The attached patch is your email in
diff form. Does this look alright?
Since you mention C++98: We emit this diagnostic in C++98 mode (before and
after my change). The rule is new in C+++11, right? Should I add a check
for CPlusPlus11 before emitting this diagnostic (in a separate change)?
I first forgot to undo my isDefaultCtor() change, and all the tests pass
both with and without it. Can you think of a test case that fails with the
isDefaultCtor() patch? (The new tests fail with _just_ the isDefaultCtor()
change.)
On Tue, Feb 23, 2016 at 2:41 PM, Richard Smith <rich...@metafoo.co.uk>
wrote:
> On Sat, Feb 20, 2016 at 6:53 AM, Nico Weber <tha...@chromium.org> wrote:
> > 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?
>
> When a constructor is added to the class, we can check whether it's a
> defaulted default constructor (that is, add another flag to
> DefinitionData to track whether there is one)[1]. Then the class has a
> defaulted default constructor if either (a) you've seen one, or (b)
> the class needs one to be implicitly declared (we already have a flag
> for that on CXXRecordDecl).
>
> So, we allow default const init if
> !(HasDeclaredDefaultedDefaultConstructor ||
> needsImplicitDefaultConstructor()) || !HasUninitializedFields.
>
> [1]: I'm not sure whether we set an implicit default constructor to
> be defaulted in C++98 mode. If not, you can check whether it's
> defaulted or implicit.
>
> >>> 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.
>
> That change is not correct :( Consider a class like this:
>
> struct X {
> template<typename ...T, typename enable_if<sizeof...(T) != 0>::type*
> = nullptr> X(T...);
> X() = default;
> };
>
> Here, the constructor template is not a default constructor. This
> class has only one default constructor, and it is defaulted.
>
Index: lib/AST/ASTImporter.cpp
===================================================================
--- lib/AST/ASTImporter.cpp (revision 261674)
+++ lib/AST/ASTImporter.cpp (working copy)
@@ -2040,6 +2040,8 @@
ToData.HasIrrelevantDestructor = FromData.HasIrrelevantDestructor;
ToData.HasConstexprNonCopyMoveConstructor
= FromData.HasConstexprNonCopyMoveConstructor;
+ ToData.HasDefaultedDefaultConstructor
+ = FromData.HasDefaultedDefaultConstructor;
ToData.DefaultedDefaultConstructorIsConstexpr
= FromData.DefaultedDefaultConstructorIsConstexpr;
ToData.HasConstexprDefaultConstructor
Index: lib/AST/DeclCXX.cpp
===================================================================
--- lib/AST/DeclCXX.cpp (revision 261674)
+++ lib/AST/DeclCXX.cpp (working copy)
@@ -61,6 +61,7 @@
DefaultedDestructorIsDeleted(false), HasTrivialSpecialMembers(SMF_All),
DeclaredNonTrivialSpecialMembers(0), HasIrrelevantDestructor(true),
HasConstexprNonCopyMoveConstructor(false),
+ HasDefaultedDefaultConstructor(false),
DefaultedDefaultConstructorIsConstexpr(true),
HasConstexprDefaultConstructor(false),
HasNonLiteralTypeFieldsOrBases(false), ComputedVisibleConversions(false),
@@ -497,6 +498,8 @@
data().UserProvidedDefaultConstructor = true;
if (Constructor->isConstexpr())
data().HasConstexprDefaultConstructor = true;
+ if (Constructor->isDefaulted())
+ data().HasDefaultedDefaultConstructor = true;
}
if (!FunTmpl) {
Index: include/clang/AST/DeclCXX.h
===================================================================
--- include/clang/AST/DeclCXX.h (revision 261674)
+++ include/clang/AST/DeclCXX.h (working copy)
@@ -421,6 +421,10 @@
/// constructor which is neither the copy nor move constructor.
bool HasConstexprNonCopyMoveConstructor : 1;
+ /// \brief True if this class has a (possibly implicit) defaulted default
+ /// constructor.
+ bool HasDefaultedDefaultConstructor : 1;
+
/// \brief True if a defaulted default constructor for this class would
/// be constexpr.
bool DefaultedDefaultConstructorIsConstexpr : 1;
@@ -1278,7 +1282,8 @@
/// per core issue 253.
bool allowConstDefaultInit() const {
return !data().HasUninitializedFields ||
- hasUserProvidedDefaultConstructor();
+ !(data().HasDefaultedDefaultConstructor ||
+ needsImplicitDefaultConstructor());
}
/// \brief Determine whether this class has a destructor which has no
Index: lib/Serialization/ASTWriter.cpp
===================================================================
--- lib/Serialization/ASTWriter.cpp (revision 261674)
+++ lib/Serialization/ASTWriter.cpp (working copy)
@@ -5558,6 +5558,7 @@
Record.push_back(Data.DeclaredNonTrivialSpecialMembers);
Record.push_back(Data.HasIrrelevantDestructor);
Record.push_back(Data.HasConstexprNonCopyMoveConstructor);
+ Record.push_back(Data.HasDefaultedDefaultConstructor);
Record.push_back(Data.DefaultedDefaultConstructorIsConstexpr);
Record.push_back(Data.HasConstexprDefaultConstructor);
Record.push_back(Data.HasNonLiteralTypeFieldsOrBases);
Index: lib/Serialization/ASTReaderDecl.cpp
===================================================================
--- lib/Serialization/ASTReaderDecl.cpp (revision 261674)
+++ lib/Serialization/ASTReaderDecl.cpp (working copy)
@@ -1423,6 +1423,7 @@
Data.DeclaredNonTrivialSpecialMembers = Record[Idx++];
Data.HasIrrelevantDestructor = Record[Idx++];
Data.HasConstexprNonCopyMoveConstructor = Record[Idx++];
+ Data.HasDefaultedDefaultConstructor = Record[Idx++];
Data.DefaultedDefaultConstructorIsConstexpr = Record[Idx++];
Data.HasConstexprDefaultConstructor = Record[Idx++];
Data.HasNonLiteralTypeFieldsOrBases = Record[Idx++];
@@ -1548,6 +1549,7 @@
OR_FIELD(DeclaredNonTrivialSpecialMembers)
MATCH_FIELD(HasIrrelevantDestructor)
OR_FIELD(HasConstexprNonCopyMoveConstructor)
+ OR_FIELD(HasDefaultedDefaultConstructor)
MATCH_FIELD(DefaultedDefaultConstructorIsConstexpr)
OR_FIELD(HasConstexprDefaultConstructor)
MATCH_FIELD(HasNonLiteralTypeFieldsOrBases)
Index: test/SemaCXX/cxx0x-cursory-default-delete.cpp
===================================================================
--- test/SemaCXX/cxx0x-cursory-default-delete.cpp (revision 261674)
+++ test/SemaCXX/cxx0x-cursory-default-delete.cpp (working copy)
@@ -74,7 +74,35 @@
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;
+};
+struct ignored_template_ctor_and_def {
+ template <class T> ignored_template_ctor_and_def(T* f = nullptr);
+ ignored_template_ctor_and_def() = default;
+ int field;
+};
+template<bool, typename = void> struct enable_if {};
+template<typename T> struct enable_if<true, T> { typedef T type; };
+struct multi_param_pack_and_defaulted {
+ template <typename... T,
+ typename enable_if<sizeof...(T) != 0>::type* = nullptr>
+ multi_param_pack_and_defaulted(T...);
+ multi_param_pack_and_defaulted() = default;
+ int n;
+};
+
void constobjs() {
const no_fields nf; // ok
const all_init ai; // ok
@@ -88,6 +116,12 @@
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
+ const multi_param_pack_and_defaulted mppad; // expected-error {{default
initialization of an object of const type 'const
multi_param_pack_and_defaulted' without a user-provided default constructor}}
+ const ignored_template_ctor_and_def itcad; // expected-error {{default
initialization of an object of const type 'const ignored_template_ctor_and_def'
without a user-provided default constructor}}
+
}
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
