Hi, On Thu, Feb 23, 2012 at 4:25 PM, David Blaikie <[email protected]> wrote:
> On Thu, Feb 23, 2012 at 1:01 PM, Nicola Gigante > <[email protected]> wrote: > > > > Il giorno 16/feb/2012, alle ore 21:21, Nicola Gigante ha scritto: > > > >> > >> Il giorno 13/feb/2012, alle ore 15:42, Nicola Gigante ha scritto: > >> > >>> > >>> Il giorno 09/feb/2012, alle ore 13:36, Nicola Gigante ha scritto: > >>>> > >>>> Forget what I said on the last mail. I was wrong. > >>>> Now I've fixed the patch. It passes all the tests an applies to > >>>> the latest revision (r150076). > >>>> If it's ok, I'd commit it :) > >>> > >>> Ping? > >>> Can I commit the patch? > >> > >> Ping^2? > >> Is anybody reviewing my patch? > > > > Ping^3? > > > > I've attached an updated patch that applies to the current > > revision, just in case. > I'm sorry it's taken so long for you to get this patch reviewed! Many thanks for your persistence. > For what it's worth, this looks like good stuff from an AST fidelity > perspective (I'm not sure I've given the actual implementation enough > review to have an opinion on whether it's the right approach, etc). > Here's one minor cleanup that we can do because of this change: > > diff --git a/lib/Sema/SemaChecking.cpp b/lib/Sema/SemaChecking.cpp > index a89e813..fe3a8a6 100644 > --- a/lib/Sema/SemaChecking.cpp > +++ b/lib/Sema/SemaChecking.cpp > @@ -4234,7 +4234,7 @@ void AnalyzeImplicitConversions(Sema &S, Expr > *OrigE, SourceLocation CC) { > > // Skip past explicit casts. > if (isa<ExplicitCastExpr>(E)) { > - E = cast<ExplicitCastExpr>(E)->getSubExpr()->IgnoreParenImpCasts(); > + E = cast<ExplicitCastExpr>(E)->getSubExpr(); > return AnalyzeImplicitConversions(S, E, CC); > } > I don't believe that's quite right; we can still see an implicit cast within an explicit cast with this patch, if two conversions are required. For instance, we should still get an implicit lvalue-to-rvalue conversion followed by an explicit integral cast in a case like: short s = 0; int n = static_cast<int>(s); Also, we presumably still want to skip parens here? On to the patch. I think it's really very close now: Index: include/clang/Sema/Sema.h =================================================================== --- include/clang/Sema/Sema.h (revision 151277) +++ include/clang/Sema/Sema.h (working copy) @@ -5837,18 +5837,44 @@ CCK_CStyleCast, /// \brief A functional-style cast. CCK_FunctionalCast, + /// \breif A static cast Typo 'breif'. Also, please add a trailing period to match the other comments on this enum. + CCK_StaticCast, /// \brief A cast other than a C-style cast. CCK_OtherCast }; - /// ImpCastExprToType - If Expr is not of type 'Type', insert an implicit - /// cast. If there is already an implicit cast, merge into the existing one. - /// If isLvalue, the result of the cast is an lvalue. + class CheckedConversionInfo { + CheckedConversionKind CCK; + SourceRange TypeRange; + TypeSourceInfo *TInfo; + + public: + explicit CheckedConversionInfo(CheckedConversionKind CCK = + CCK_ImplicitConversion, + SourceRange TypeRange = SourceRange(), + TypeSourceInfo *TInfo = 0) + : CCK(CCK), TypeRange(TypeRange), TInfo(TInfo) { } Please indent this ':' more deeply than the declaration of the constructor. Index: include/clang/Sema/Initialization.h =================================================================== --- include/clang/Sema/Initialization.h (revision 151277) +++ include/clang/Sema/Initialization.h (working copy) @@ -390,10 +390,16 @@ /// \brief The source locations involved in the initialization. SourceLocation Locations[3]; + + /// \brief The source info of the initialization + TypeSourceInfo *TInfo; Trailing period in this comment too, please. /// \brief Create a direct initialization due to a cast that isn't a C-style /// or functional cast. - static InitializationKind CreateCast(SourceRange TypeRange) { - return InitializationKind(IK_Direct, IC_StaticCast, TypeRange.getBegin(), - TypeRange.getBegin(), TypeRange.getEnd()); + static InitializationKind CreateStaticCast(Sema::CheckedConversionInfo CCI) { + return InitializationKind(IK_Direct, IC_StaticCast, + CCI.getTypeRange().getBegin(), + CCI.getTypeRange().getBegin(), + CCI.getTypeRange().getEnd(), + CCI.getTypeSourceInfo()); This comment doesn't match the function name: reinterpret_cast and dynamic_cast are not C-style nor functional, but also aren't static casts. @@ -719,7 +739,8 @@ /// CheckStaticCast - Check that a static_cast\<DestType\>(SrcExpr) is valid. /// Refer to C++ 5.2.9 for details. Static casts are mostly used for making /// implicit conversions explicit and getting rid of data loss warnings. This comment should be updated to indicate that this function (sometimes) creates a CXXStaticCast expression. -void CastOperation::CheckStaticCast() { +void CastOperation::CheckStaticCast(bool &CastNodesCreated, + Sema::CheckedConversionInfo CCI) { Rather than taking a bool argument by reference, it might be nicer to return a bool from here to indicate whether a CXXStaticCast node was created. + CastNodesCreated = (SrcExpr.get() != SrcExprOrig && + Kind != CK_ConstructorConversion); This approach makes me nervous: it seems too easy for us to accidentally change SrcExpr without building a cast node (checkNonOverloadPlaceholders could do this, for instance). Can we ensure that TryStaticCast returns TC_Success exactly when it's created a CXXStaticCast node, then use that to determine whether we need to build one? These last few comments also apply to the CXXCStyleCast case. Index: lib/Sema/SemaExprCXX.cpp =================================================================== --- lib/Sema/SemaExprCXX.cpp (revision 151277) +++ lib/Sema/SemaExprCXX.cpp (working copy) @@ -2670,29 +2671,30 @@ // _Complex x -> x From = ImpCastExprToType(From, ElType, - isFloatingComplex ? CK_FloatingComplexToReal - : CK_IntegralComplexToReal, - VK_RValue, /*BasePath=*/0, CCK).take(); + isFloatingComplex ? CK_FloatingComplexToReal + : CK_IntegralComplexToReal).take(); // x -> y if (Context.hasSameUnqualifiedType(ElType, ToType)) { // do nothing } else if (ToType->isRealFloatingType()) { - From = ImpCastExprToType(From, ToType, - isFloatingComplex ? CK_FloatingCast : CK_IntegralToFloating, - VK_RValue, /*BasePath=*/0, CCK).take(); + From = CastExprToType(From, ToType, + isFloatingComplex ? CK_FloatingCast + : CK_IntegralToFloating, + VK_RValue, /*BasePath=*/0, CCI).take(); } else { assert(ToType->isIntegerType()); - From = ImpCastExprToType(From, ToType, - isFloatingComplex ? CK_FloatingToIntegral : CK_IntegralCast, - VK_RValue, /*BasePath=*/0, CCK).take(); + From = CastExprToType(From, ToType, + isFloatingComplex ? CK_FloatingToIntegral + : CK_IntegralCast, + VK_RValue, /*BasePath=*/0, CCI).take(); } } break; In the ElType == ToType case, it looks like this could still produce a no-op static cast containing an implicit cast. Thanks! Richard
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
