r261297 - Implement the likely resolution of core issue 253.
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Wed Feb 24 12:53:57 PST 2016
On Wed, Feb 24, 2016 at 10:47 AM, Nico Weber <thakis at 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.
> 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), "");
> On Tue, Feb 23, 2016 at 2:41 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
>>
>> On Sat, Feb 20, 2016 at 6:53 AM, Nico Weber <thakis at chromium.org> wrote:
>> > On Fri, Feb 19, 2016 at 10:32 PM, Nico Weber <thakis at chromium.org>
>> > wrote:
>> >>
>> >> 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?
>>
>> 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.
>
>
More information about the cfe-commits
mailing list