[cfe-commits] [Patch] Implement compiler intrinsics for MSVC 2012 type_traits
Ryan Molden
ryanmolden at gmail.com
Sat Nov 17 11:05:59 PST 2012
On Fri, Nov 16, 2012 at 6:21 PM, Richard Smith <richard at metafoo.co.uk>wrote:
> On Sun, Nov 11, 2012 at 10:38 AM, Ryan Molden <ryanmolden at gmail.com>
> wrote:
> >
> >
> > On Fri, Nov 9, 2012 at 9:13 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
> >>
> >> On Thu, Nov 8, 2012 at 5:49 PM, Ryan Molden <ryanmolden at gmail.com>
> wrote:
> >> > This is a re-submission of an older proposed patch
> >> >
> >> > (
> http://www.mail-archive.com/cfe-commits@cs.uiuc.edu/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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121117/ead4a8c8/attachment.html>
More information about the cfe-commits
mailing list