On Sat, Nov 17, 2012 at 11:05 AM, Ryan Molden <[email protected]> wrote:
> On Fri, Nov 16, 2012 at 6:21 PM, Richard Smith <[email protected]>wrote: > >> On Sun, Nov 11, 2012 at 10:38 AM, Ryan Molden <[email protected]> >> wrote: >> > >> > >> > On Fri, Nov 9, 2012 at 9:13 PM, Richard Smith <[email protected]> >> wrote: >> >> >> >> On Thu, Nov 8, 2012 at 5:49 PM, Ryan Molden <[email protected]> >> wrote: >> >> > This is a re-submission of an older proposed patch >> >> > >> >> > ( >> http://www.mail-archive.com/[email protected]/msg55616/0001-Added-support-for-MSVC-2012-type-traits-used-in-stan.patch >> ) >> >> > that João hadn't had time to write tests for (which were requested >> with >> >> > the >> >> > original submission review). >> >> > >> >> > The only changes I made from the original (apart from adding tests) >> was >> >> > to >> >> > take out the bail-out for hasTrivialMoveAssignment from >> >> > UTT_HasNothrowMoveAssign in EvaluateUnaryTypeTrait (in >> >> > lib\Sema\SemaExprCXX.cpp). >> >> > >> >> > My reasoning was that trivial move assignment operators (which I >> >> > understand >> >> > to be implicitly generated ones, please correct me if this is >> mistaken) >> >> > can >> >> > actually have non-empty exception specifiers if any of the member >> >> > move-assignment operators they invoke have such non-empty exception >> >> > specifiers. >> >> > >> >> > Specifically: >> >> > >> >> > n3376 15.4 [except.spec]/14 >> >> > >> >> > An inheriting constructor (12.9) and an implicitly declared special >> >> > member >> >> > function (Clause 12) have an exception-specification. If f is an >> >> > inheriting >> >> > constructor or an implicitly declared default constructor, copy >> >> > constructor, >> >> > move constructor, destructor, copy assignment operator, or move >> >> > assignment >> >> > operator, its implicit exception-specification specifies the type-id >> T >> >> > if >> >> > and only if T is allowed by the exception-specification of a function >> >> > directly invoked by f’s implicit definition; f allows all exceptions >> if >> >> > any >> >> > function it directly invokes allows all exceptions, and f has the >> >> > exception-specification noexcept(true) if every function it directly >> >> > invokes >> >> > allows no exceptions. [ Note: An instantiation of an inheriting >> >> > constructor >> >> > template has an implied exception-specification as if it were a >> >> > non-template >> >> > inheriting constructor.] >> >> > >> >> > so I would expect this class (HasMemberThrowMoveAssign) to fail for >> >> > std::is_nothrow_move_assignable: >> >> > >> >> > struct NonPOD { NonPOD(int); }; enum Enum { EV }; struct POD { Enum >> e; >> >> > int >> >> > i; float f; NonPOD* p; }; >> >> > >> >> > struct HasThrowMoveAssign { HasThrowMoveAssign& operator =(const >> >> > HasThrowMoveAssign&&) throw(POD); }; >> >> > struct HasMemberThrowMoveAssign { HasThrowMoveAssign member; }; >> >> > >> >> > even though it should have a trivial move-assignment operator >> generated. >> >> > Please correct me if I am mistaken here as my standards reading FU >> >> > is...not >> >> > strong. >> >> >> >> You are mistaken here ;-) >> >> >> >> HasMemberThrowMoveAssign's move assignment is not trivial because it >> >> calls a non-trivial move assignment operator. It is possible to have a >> >> throwing trivial move assignment operator, but only if it is deleted. >> >> In that case, the trait should presumbly return false. >> > >> > >> > Great, thanks for the catch. So it seems that having the early bail-out >> for >> > things with trivial move assignment operators is correct. I put it back >> in >> > and all tests are still passing (I thought one of them was failing >> before I >> > took it out, but that was a week+ ago, so perhaps I am just >> > mis-remembering). >> > >> > It isn't clear how I would make a 'throwing trivial move assignment >> > operator', if you have suggestions I can certainly add a test for it. >> >> This class has a throwing trivial move assignment operator: >> >> struct S { >> S &operator=(const S&) throw(int) = delete; >> }; >> >> > New patch attached that simply syncs to tip and adds back the bail-out >> for >> > types with a trivial move-assignment operator. >> >> I'm surprised that there's no __has_nothrow_move_constructor and >> __has_nothrow_destructor. It's also surprising that we have >> __has_trivial_copy for the copy ctor but >> __has_trivial_move_constructor for the move ctor. I assume this >> matches MSVC? >> >> The patch itself generally looks fine, apart from some coding style >> issues: >> >> Index: lib/AST/StmtPrinter.cpp >> =================================================================== >> - case UTT_HasTrivialDefaultConstructor: return >> "__has_trivial_constructor"; >> + case UTT_HasTrivialMoveAssign: return "__has_trivial_move_assign"; >> + case UTT_HasTrivialMoveConstructor: return >> "__has_trivial_move_constructor"; >> + case UTT_HasTrivialDefaultConstructor: return >> "__has_trivial_constructor"; >> >> Some trailing whitespace has been added here, please remove :) >> >> Index: lib/Parse/ParseExpr.cpp >> =================================================================== >> --- lib/Parse/ParseExpr.cpp (revision 167691) >> +++ lib/Parse/ParseExpr.cpp (working copy) >> @@ -1241,8 +1241,11 @@ >> case tok::kw___is_union: >> case tok::kw___is_final: >> case tok::kw___has_trivial_constructor: >> + case tok::kw___has_trivial_move_constructor: >> case tok::kw___has_trivial_copy: >> case tok::kw___has_trivial_assign: >> + case tok::kw___has_nothrow_move_assign: >> + case tok::kw___has_trivial_move_assign: >> case tok::kw___has_trivial_destructor: >> case tok::kw___has_nothrow_assign: >> case tok::kw___has_nothrow_copy: >> >> __has_nothrow_move_assign is not in the right order here. >> >> Index: lib/Sema/SemaExprCXX.cpp >> =================================================================== >> @@ -3071,6 +3074,16 @@ >> C.getBaseElementType(T)->getAs<RecordType>()) >> return >> cast<CXXRecordDecl>(RT->getDecl())->hasTrivialDefaultConstructor(); >> return false; >> + case UTT_HasTrivialMoveConstructor: >> + // This trait is implemented by MSVC 2012 and needed to parse the >> + // standard library headers. Specifically this is used as the logic >> + // behind std::has_trivial_move_constructor (20.9.4.3). >> + if (T.isPODType(Self.Context)) >> + return true; >> + if (const RecordType *RT = >> + C.getBaseElementType(T)->getAs<RecordType>()) >> + return >> cast<CXXRecordDecl>(RT->getDecl())->hasTrivialMoveConstructor(); >> + return false; >> >> Unnecessary line break here. >> >> + case UTT_HasNothrowMoveAssign: >> + // This trait is implemented by MSVC 2012 and needed to parse the >> + // standard library headers. Specifically this is used as the logic >> + // behind std::is_nothrow_move_assignable (20.9.4.3). >> + if (T.isPODType(Self.Context)) >> + return true; >> + >> + if (const RecordType *RT = >> C.getBaseElementType(T)->getAs<RecordType>()) { >> + CXXRecordDecl* RD = cast<CXXRecordDecl>(RT->getDecl()); >> + if (RD->hasTrivialMoveAssignment()) >> + return true; >> + >> + bool FoundAssign = false; >> + DeclarationName Name = >> C.DeclarationNames.getCXXOperatorName(OO_Equal); >> + LookupResult Res(Self, DeclarationNameInfo(Name, KeyLoc), >> Sema::LookupOrdinaryName); >> + if (Self.LookupQualifiedName(Res, RD)) { >> + Res.suppressDiagnostics(); >> + for (LookupResult::iterator Op = Res.begin(), OpEnd = >> Res.end(); Op != OpEnd; ++Op) { >> + if (isa<FunctionTemplateDecl>(*Op)) >> + continue; >> + >> + CXXMethodDecl *Operator = cast<CXXMethodDecl>(*Op); >> + if (Operator->isMoveAssignmentOperator()) { >> + FoundAssign = true; >> + const FunctionProtoType *CPT = >> Operator->getType()->getAs<FunctionProtoType>(); >> + CPT = Self.ResolveExceptionSpec(KeyLoc, CPT); >> + if (!CPT) >> + return false; >> + if (!CPT->isNothrow(Self.Context)) >> + return false; >> + } >> + } >> + } >> + >> + return FoundAssign; >> + } >> + return false; >> >> Some lines here are over the 80 column limit. >> > > >I'm surprised that there's no __has_nothrow_move_constructor and > __has_nothrow_destructor. It's also surprising that we have > __has_trivial_copy for the copy ctor but > __has_trivial_move_constructor for the move ctor. I assume this > matches MSVC? > > You and your 'we should be exhaustive in our coverage' :) I believe there > are more, the ones above were simply the ones I ran into while trying to > use Clang to process some code. I believe the ones you mention (like > __has_nothrow_move_constructor) are currently hiding behind other errors > (specifically Clang's intense dislike of the VC STL variadac template > 'emulating' macros). I will try and do a search of the type_traits header > looking for all the compiler intrinsics they rely on and cross-referencing > with what Clang currently has, adding any that aren't there. > > I will also address the other comments, thanks much! > > Ryan > Okay, I was mistaken, it looks like Clang covers all intrinsics necessary (after my patch). The spreadsheet is here ( https://docs.google.com/spreadsheet/ccc?key=0Avvye004lP64dG5jU1ZkWWpLM3ZqOVlWYWgtV1RBV3c) with my investigative results on this. There are a couple of type traits that MSVC 2012 has marked as approximations. Currently these don't require intrinsics, eventually I suspect they will, but since we don't know for sure what MSVC will call them it doesn't seem to make sense to try and pre-emptively include them, so I am not going to bother there. >I'm surprised that there's no __has_nothrow_move_constructor they don't have a __has_nothrow_move_constructor, I presume it would be used for something like std::is_nothrow_move_constructible, but that is defined in their headers as: // TEMPLATE CLASS is_nothrow_move_constructible template<class _Ty> struct is_nothrow_move_constructible : _Cat_base<!is_array<_Ty>::value && is_nothrow_constructible< typename remove_volatile<_Ty>::type, typename add_rvalue_reference< typename remove_volatile<_Ty>::type>::type>::value> { // determine whether _Ty has a nothrow move constructor }; and is_nothrow_constructible utilizes __has_nothrow_constructor, which already existed in Clang before this patch. >and __has_nothrow_destructor. There is_nothrow_destructible is marked as approximation and is set to derive from true_type at the moment. They will likely need something like __has_nothrow_destructor to implement this fully, but until they release an STL relying on that intrinsic adding it to Clang seems premature. >It's also surprising that we have __has_trivial_copy for the copy ctor but __has_trivial_move_constructor for the move ctor. I assume this matches MSVC? __has_trivial_move_constructor is the instrinc that MSVC STL expects to exist. It looks like __has_trivial_copy is likely for GNU supports, not MSVC. MSVC relies on __is_trivially_copyable intrinsic, which existed in Clang before this changeset. New patch attached, cleanup various formatting issues from above + add some more unit tests around deleted functions. Ryan
Support-For-MSVC-2012-Type-Traits-For-STL.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
