[cfe-commits] [cfe-dev] [review request] Removing redundant implicit casts in the AST, take 2
David Blaikie
dblaikie at gmail.com
Thu Feb 23 18:42:17 PST 2012
On Thu, Feb 23, 2012 at 6:28 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> Hi,
>
> On Thu, Feb 23, 2012 at 4:25 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> On Thu, Feb 23, 2012 at 1:01 PM, Nicola Gigante
>> <nicola.gigante at gmail.com> 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?
Sorry, I'll provide some more context - AnalyzeImplicitConversions
itself does "IgnoreParensImpCasts" as its first thing - but not before
recording the QualType of the outer Expr (lines 4202, 4203). Many of
the diagnostics work simply by comparing the type of the outer
expression with the type of the inner one - thus they fired /all/ if
you didn't have that IgnoreParensImpCasts before the
AnalyzeImplicitConversions call - because they looked just like
implicit conversions (they /were/ in the AST). Now that explicit
conversions aren't implemented as implicit ones this problem doesn't
exist.
In fact when I made this change I was hoping there were cases where an
explicit conversion could contain an implicit one as you showed - but
further, that such cases might have real warnings. Turned out removing
that outer IgnoreParensImpCasts didn't cause any clang tests to change
at all... nice, even if it didn't allow us to catch new cases.
In any case I suspect it's the right thing - there's no reason for us
to be ignoring those implicit conversions except that we were doing it
to support our not-so-great AST representation of explicit
conversions.
>
>
> 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
More information about the cfe-commits
mailing list