On Fri, Nov 16, 2012 at 6:21 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">On Sun, Nov 11, 2012 at 10:38 AM, Ryan Molden <<a href="mailto:ryanmolden@gmail.com">ryanmolden@gmail.com</a>> wrote:<br>
><br>
><br>
> On Fri, Nov 9, 2012 at 9:13 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
>><br>
>> On Thu, Nov 8, 2012 at 5:49 PM, Ryan Molden <<a href="mailto:ryanmolden@gmail.com">ryanmolden@gmail.com</a>> wrote:<br>
>> > This is a re-submission of an older proposed patch<br>
>> ><br>
>> > (<a href="http://www.mail-archive.com/cfe-commits@cs.uiuc.edu/msg55616/0001-Added-support-for-MSVC-2012-type-traits-used-in-stan.patch" target="_blank">http://www.mail-archive.com/cfe-commits@cs.uiuc.edu/msg55616/0001-Added-support-for-MSVC-2012-type-traits-used-in-stan.patch</a>)<br>
>> > that João hadn't had time to write tests for (which were requested with<br>
>> > the<br>
>> > original submission review).<br>
>> ><br>
>> > The only changes I made from the original (apart from adding tests) was<br>
>> > to<br>
>> > take out the bail-out for hasTrivialMoveAssignment from<br>
>> > UTT_HasNothrowMoveAssign in EvaluateUnaryTypeTrait (in<br>
>> > lib\Sema\SemaExprCXX.cpp).<br>
>> ><br>
>> > My reasoning was that trivial move assignment operators (which I<br>
>> > understand<br>
>> > to be implicitly generated ones, please correct me if this is mistaken)<br>
>> > can<br>
>> > actually have non-empty exception specifiers if any of the member<br>
>> > move-assignment operators they invoke have such non-empty exception<br>
>> > specifiers.<br>
>> ><br>
>> > Specifically:<br>
>> ><br>
>> > n3376 15.4 [except.spec]/14<br>
>> ><br>
>> > An inheriting constructor (12.9) and an implicitly declared special<br>
>> > member<br>
>> > function (Clause 12) have an exception-specification. If f is an<br>
>> > inheriting<br>
>> > constructor or an implicitly declared default constructor, copy<br>
>> > constructor,<br>
>> > move constructor, destructor, copy assignment operator, or move<br>
>> > assignment<br>
>> > operator, its implicit exception-specification specifies the type-id T<br>
>> > if<br>
>> > and only if T is allowed by the exception-specification of a function<br>
>> > directly invoked by f’s implicit definition; f allows all exceptions if<br>
>> > any<br>
>> > function it directly invokes allows all exceptions, and f has the<br>
>> > exception-specification noexcept(true) if every function it directly<br>
>> > invokes<br>
>> > allows no exceptions. [ Note: An instantiation of an inheriting<br>
>> > constructor<br>
>> > template has an implied exception-specification as if it were a<br>
>> > non-template<br>
>> > inheriting constructor.]<br>
>> ><br>
>> > so I would expect this class (HasMemberThrowMoveAssign) to fail for<br>
>> > std::is_nothrow_move_assignable:<br>
>> ><br>
>> > struct NonPOD { NonPOD(int); }; enum Enum { EV }; struct POD { Enum e;<br>
>> > int<br>
>> > i; float f; NonPOD* p; };<br>
>> ><br>
>> > struct HasThrowMoveAssign { HasThrowMoveAssign& operator =(const<br>
>> > HasThrowMoveAssign&&) throw(POD); };<br>
>> > struct HasMemberThrowMoveAssign { HasThrowMoveAssign member; };<br>
>> ><br>
>> > even though it should have a trivial move-assignment operator generated.<br>
>> > Please correct me if I am mistaken here as my standards reading FU<br>
>> > is...not<br>
>> > strong.<br>
>><br>
>> You are mistaken here ;-)<br>
>><br>
>> HasMemberThrowMoveAssign's move assignment is not trivial because it<br>
>> calls a non-trivial move assignment operator. It is possible to have a<br>
>> throwing trivial move assignment operator, but only if it is deleted.<br>
>> In that case, the trait should presumbly return false.<br>
><br>
><br>
> Great, thanks for the catch. So it seems that having the early bail-out for<br>
> things with trivial move assignment operators is correct. I put it back in<br>
> and all tests are still passing (I thought one of them was failing before I<br>
> took it out, but that was a week+ ago, so perhaps I am just<br>
> mis-remembering).<br>
><br>
> It isn't clear how I would make a 'throwing trivial move assignment<br>
> operator', if you have suggestions I can certainly add a test for it.<br>
<br>
</div></div>This class has a throwing trivial move assignment operator:<br>
<br>
struct S {<br>
S &operator=(const S&) throw(int) = delete;<br>
<div class="im">};<br>
<br>
> New patch attached that simply syncs to tip and adds back the bail-out for<br>
> types with a trivial move-assignment operator.<br>
<br>
</div>I'm surprised that there's no __has_nothrow_move_constructor and<br>
__has_nothrow_destructor. It's also surprising that we have<br>
__has_trivial_copy for the copy ctor but<br>
__has_trivial_move_constructor for the move ctor. I assume this<br>
matches MSVC?<br>
<br>
The patch itself generally looks fine, apart from some coding style issues:<br>
<br>
Index: lib/AST/StmtPrinter.cpp<br>
===================================================================<br>
- case UTT_HasTrivialDefaultConstructor: return "__has_trivial_constructor";<br>
+ case UTT_HasTrivialMoveAssign: return "__has_trivial_move_assign";<br>
+ case UTT_HasTrivialMoveConstructor: return "__has_trivial_move_constructor";<br>
+ case UTT_HasTrivialDefaultConstructor: return "__has_trivial_constructor";<br>
<br>
Some trailing whitespace has been added here, please remove :)<br>
<br>
Index: lib/Parse/ParseExpr.cpp<br>
===================================================================<br>
--- lib/Parse/ParseExpr.cpp (revision 167691)<br>
+++ lib/Parse/ParseExpr.cpp (working copy)<br>
@@ -1241,8 +1241,11 @@<br>
case tok::kw___is_union:<br>
case tok::kw___is_final:<br>
case tok::kw___has_trivial_constructor:<br>
+ case tok::kw___has_trivial_move_constructor:<br>
case tok::kw___has_trivial_copy:<br>
case tok::kw___has_trivial_assign:<br>
+ case tok::kw___has_nothrow_move_assign:<br>
+ case tok::kw___has_trivial_move_assign:<br>
case tok::kw___has_trivial_destructor:<br>
case tok::kw___has_nothrow_assign:<br>
case tok::kw___has_nothrow_copy:<br>
<br>
__has_nothrow_move_assign is not in the right order here.<br>
<br>
Index: lib/Sema/SemaExprCXX.cpp<br>
===================================================================<br>
@@ -3071,6 +3074,16 @@<br>
C.getBaseElementType(T)->getAs<RecordType>())<br>
return cast<CXXRecordDecl>(RT->getDecl())->hasTrivialDefaultConstructor();<br>
return false;<br>
+ case UTT_HasTrivialMoveConstructor:<br>
+ // This trait is implemented by MSVC 2012 and needed to parse the<br>
+ // standard library headers. Specifically this is used as the logic<br>
+ // behind std::has_trivial_move_constructor (20.9.4.3).<br>
+ if (T.isPODType(Self.Context))<br>
+ return true;<br>
+ if (const RecordType *RT =<br>
+ C.getBaseElementType(T)->getAs<RecordType>())<br>
+ return cast<CXXRecordDecl>(RT->getDecl())->hasTrivialMoveConstructor();<br>
+ return false;<br>
<br>
Unnecessary line break here.<br>
<br>
+ case UTT_HasNothrowMoveAssign:<br>
+ // This trait is implemented by MSVC 2012 and needed to parse the<br>
+ // standard library headers. Specifically this is used as the logic<br>
+ // behind std::is_nothrow_move_assignable (20.9.4.3).<br>
+ if (T.isPODType(Self.Context))<br>
+ return true;<br>
+<br>
+ if (const RecordType *RT = C.getBaseElementType(T)->getAs<RecordType>()) {<br>
+ CXXRecordDecl* RD = cast<CXXRecordDecl>(RT->getDecl());<br>
+ if (RD->hasTrivialMoveAssignment())<br>
+ return true;<br>
+<br>
+ bool FoundAssign = false;<br>
+ DeclarationName Name = C.DeclarationNames.getCXXOperatorName(OO_Equal);<br>
+ LookupResult Res(Self, DeclarationNameInfo(Name, KeyLoc),<br>
Sema::LookupOrdinaryName);<br>
+ if (Self.LookupQualifiedName(Res, RD)) {<br>
+ Res.suppressDiagnostics();<br>
+ for (LookupResult::iterator Op = Res.begin(), OpEnd =<br>
Res.end(); Op != OpEnd; ++Op) {<br>
+ if (isa<FunctionTemplateDecl>(*Op))<br>
+ continue;<br>
+<br>
+ CXXMethodDecl *Operator = cast<CXXMethodDecl>(*Op);<br>
+ if (Operator->isMoveAssignmentOperator()) {<br>
+ FoundAssign = true;<br>
+ const FunctionProtoType *CPT =<br>
Operator->getType()->getAs<FunctionProtoType>();<br>
+ CPT = Self.ResolveExceptionSpec(KeyLoc, CPT);<br>
+ if (!CPT)<br>
+ return false;<br>
+ if (!CPT->isNothrow(Self.Context))<br>
+ return false;<br>
+ }<br>
+ }<br>
+ }<br>
+<br>
+ return FoundAssign;<br>
+ }<br>
+ return false;<br>
<br>
Some lines here are over the 80 column limit.<br>
</blockquote></div><div><br></div>>I'm surprised that there's no __has_nothrow_move_constructor and<br>__has_nothrow_destructor. It's also surprising that we have<br>__has_trivial_copy for the copy ctor but<br>
__has_trivial_move_constructor for the move ctor. I assume this<br>matches MSVC?<br><div><br></div><div>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.</div>
<div><br></div><div>I will also address the other comments, thanks much!</div><div><br></div><div>Ryan</div>