On Sat, Nov 17, 2012 at 11:05 AM, Ryan Molden <span dir="ltr"><<a href="mailto:ryanmolden@gmail.com" target="_blank">ryanmolden@gmail.com</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><div>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><div>On Sun, Nov 11, 2012 at 10:38 AM, Ryan Molden <<a href="mailto:ryanmolden@gmail.com" target="_blank">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" target="_blank">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" target="_blank">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>};<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></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><span><font color="#888888"><div><br></div><div>Ryan</div>
</font></span></blockquote></div><br><div>Okay, I was mistaken, it looks like Clang covers all intrinsics necessary (after my patch). The spreadsheet is here (<a href="https://docs.google.com/spreadsheet/ccc?key=0Avvye004lP64dG5jU1ZkWWpLM3ZqOVlWYWgtV1RBV3c" target="_blank">https://docs.google.com/spreadsheet/ccc?key=0Avvye004lP64dG5jU1ZkWWpLM3ZqOVlWYWgtV1RBV3c</a>) with my investigative results on this.</div>
<div><br></div><div>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.</div>
<div><br></div><div>>I'm surprised that there's no __has_nothrow_move_constructor</div><div><br></div><div>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:</div>
<div><br></div><div>
<div>// TEMPLATE CLASS is_nothrow_move_constructible</div><div>template<class _Ty></div><div>struct is_nothrow_move_constructible</div><div> : _Cat_base<!is_array<_Ty>::value</div><div> && is_nothrow_constructible<</div>
<div><span style="white-space:pre-wrap"> </span>typename remove_volatile<_Ty>::type,</div><div><span style="white-space:pre-wrap"> </span>typename add_rvalue_reference<</div>
<div><span style="white-space:pre-wrap"> </span> typename remove_volatile<_Ty>::type>::type>::value></div><div>{<span style="white-space:pre-wrap"> </span>// determine whether _Ty has a nothrow move constructor</div>
<div>};</div></div><div><br></div><div>and is_nothrow_constructible utilizes __has_nothrow_constructor, which already existed in Clang before this patch.</div><div><br></div><div>>and __has_nothrow_destructor. </div><div>
<br></div><div><div>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.</div>
<div> </div></div><div><div>>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?</div></div><div><br></div><div><div>
__has_trivial_move_constructor is the instrinc that MSVC STL expects to exist.</div></div><div><br></div><div>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. </div>
<div><br></div><div>New patch attached, cleanup various formatting issues from above + add some more unit tests around deleted functions.</div>
<div><br></div><div>Ryan</div>