r261297 - Implement the likely resolution of core issue 253.
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 23 11:41:24 PST 2016
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