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
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
