On Wed, Feb 24, 2016 at 3:53 PM, Richard Smith <rich...@metafoo.co.uk>
wrote:
> On Wed, Feb 24, 2016 at 10:47 AM, Nico Weber <tha...@chromium.org> wrote:
> > Thanks for patiently explaining this. The attached patch is your email in
> > diff form. Does this look alright?
>
> Yes, it looks great. Thanks for the excellent test cases.
>
r261770, thanks!
>
> > 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)?
>
> The rule was present (although worded differently), in paragraph 9:
> "If no initializer is specified for an object, and the object is of
> (possibly cv-qualified) non-POD class type (or array thereof), the
> object shall be default-initialized; if the object is of
> const-qualified type, the underlying class type shall have a
> user-declared default constructor."
>
> > 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.)
>
> Looking through the uses, there seem to be roughly four different
> things going on:
>
> 1) Checks for a trivial default constructor. These are unaffected by
> your change because a templated default constructor can never be
> trivial.
> 2) Checks for a default constructor for diagnostic purposes. I think
> they'd be surprised if we called a constructor template a default
> constructor, especially if it's being passed arguments (the same is
> true in the default argument case, though).
> 3) Checks for a default constructor for type traits and the like.
> These aren't really all that meaningful, but we probably shouldn't
> change their outcomes (we're emlating GCC behavior).
> 4) A very small number of language semantic checks; the ones whose
> outcomes change seem to mostly be wrong (they want to find the
> constructor that overload resolution would actually select for a
> 0-argument call).
>
> The easiest way to test this would probably be to static_assert that a
> class with a constructor template returns false for __is_trivial:
>
> struct X { template<typename ...T> X(T...); };
> static_assert(!__is_trivial(X), "");
>
I had tried something like that (I think it was the
`static_assert(__has_nothrow_constructor(X), "");` upthread), but I can't
manage to behave it differently by changing isDefaultConstructor(). A
mystery for another day, I suppose :-) Thanks again!
>
> > 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.
> >
> >
>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
