<div class="gmail_quote">Hi,</div><div class="gmail_quote"><br></div><div class="gmail_quote">On Thu, Feb 23, 2012 at 4:25 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>></span> wrote:<br>
<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 Thu, Feb 23, 2012 at 1:01 PM, Nicola Gigante<br>
<<a href="mailto:nicola.gigante@gmail.com">nicola.gigante@gmail.com</a>> wrote:<br>
><br>
> Il giorno 16/feb/2012, alle ore 21:21, Nicola Gigante ha scritto:<br>
><br>
>><br>
>> Il giorno 13/feb/2012, alle ore 15:42, Nicola Gigante ha scritto:<br>
>><br>
>>><br>
>>> Il giorno 09/feb/2012, alle ore 13:36, Nicola Gigante ha scritto:<br>
>>>><br>
>>>> Forget what I said on the last mail. I was wrong.<br>
>>>> Now I've fixed the patch. It passes all the tests an applies to<br>
>>>> the latest revision (r150076).<br>
>>>> If it's ok, I'd commit it :)<br>
>>><br>
>>> Ping?<br>
>>> Can I commit the patch?<br>
>><br>
>> Ping^2?<br>
>> Is anybody reviewing my patch?<br>
><br>
> Ping^3?<br>
><br>
> I've attached an updated patch that applies to the current<br>
> revision, just in case.<br></div></div></blockquote><div><br></div><div>I'm sorry it's taken so long for you to get this patch reviewed! Many thanks for your persistence.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">
</div></div>For what it's worth, this looks like good stuff from an AST fidelity<br>
perspective (I'm not sure I've given the actual implementation enough<br>
review to have an opinion on whether it's the right approach, etc).<br>
Here's one minor cleanup that we can do because of this change:<br>
<br>
diff --git a/lib/Sema/SemaChecking.cpp b/lib/Sema/SemaChecking.cpp<br>
index a89e813..fe3a8a6 100644<br>
--- a/lib/Sema/SemaChecking.cpp<br>
+++ b/lib/Sema/SemaChecking.cpp<br>
@@ -4234,7 +4234,7 @@ void AnalyzeImplicitConversions(Sema &S, Expr<br>
*OrigE, SourceLocation CC) {<br>
<br>
// Skip past explicit casts.<br>
if (isa<ExplicitCastExpr>(E)) {<br>
- E = cast<ExplicitCastExpr>(E)->getSubExpr()->IgnoreParenImpCasts();<br>
+ E = cast<ExplicitCastExpr>(E)->getSubExpr();<br>
return AnalyzeImplicitConversions(S, E, CC);<br>
}<br></blockquote><div><br></div><div>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:</div>
<div><br></div><div> short s = 0;</div><div> int n = static_cast<int>(s);</div><div><br></div><div>Also, we presumably still want to skip parens here?</div><div><br></div><div><br></div><div>On to the patch. I think it's really very close now:</div>
<div><br></div><div><div>Index: include/clang/Sema/Sema.h</div><div>===================================================================</div><div>--- include/clang/Sema/Sema.h<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 151277)</div>
<div>+++ include/clang/Sema/Sema.h<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -5837,18 +5837,44 @@</div><div> CCK_CStyleCast,</div><div> /// \brief A functional-style cast.</div>
<div> CCK_FunctionalCast,</div><div>+ /// \breif A static cast</div><div><br></div><div>Typo 'breif'. Also, please add a trailing period to match the other comments on this enum.</div><div><br></div><div>+ CCK_StaticCast,</div>
<div> /// \brief A cast other than a C-style cast.</div><div> CCK_OtherCast</div><div> };</div><div> </div><div>- /// ImpCastExprToType - If Expr is not of type 'Type', insert an implicit</div><div>- /// cast. If there is already an implicit cast, merge into the existing one.</div>
<div>- /// If isLvalue, the result of the cast is an lvalue.</div><div>+ class CheckedConversionInfo {</div><div>+ CheckedConversionKind CCK;</div><div>+ SourceRange TypeRange;</div><div>+ TypeSourceInfo *TInfo;</div>
<div>+ </div><div>+ public: </div><div>+ explicit CheckedConversionInfo(CheckedConversionKind CCK =</div><div>+ CCK_ImplicitConversion,</div><div>+ SourceRange TypeRange = SourceRange(),</div>
<div>+ TypeSourceInfo *TInfo = 0)</div><div>+ : CCK(CCK), TypeRange(TypeRange), TInfo(TInfo) { }</div></div><div><br></div><div>Please indent this ':' more deeply than the declaration of the constructor.</div>
<div><br></div><div><div>Index: include/clang/Sema/Initialization.h</div><div>===================================================================</div><div>--- include/clang/Sema/Initialization.h<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 151277)</div>
<div>+++ include/clang/Sema/Initialization.h<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div></div><div><div>@@ -390,10 +390,16 @@</div><div> </div><div> /// \brief The source locations involved in the initialization.</div>
<div> SourceLocation Locations[3];</div><div>+ </div><div>+ /// \brief The source info of the initialization</div><div>+ TypeSourceInfo *TInfo;</div></div><div><br></div><div>Trailing period in this comment too, please.</div>
<div><br></div><div><div> /// \brief Create a direct initialization due to a cast that isn't a C-style </div><div> /// or functional cast.</div><div>- static InitializationKind CreateCast(SourceRange TypeRange) {</div>
<div>- return InitializationKind(IK_Direct, IC_StaticCast, TypeRange.getBegin(),</div><div>- TypeRange.getBegin(), TypeRange.getEnd());</div><div>+ static InitializationKind CreateStaticCast(Sema::CheckedConversionInfo CCI) {</div>
<div>+ return InitializationKind(IK_Direct, IC_StaticCast,</div><div>+ CCI.getTypeRange().getBegin(),</div><div>+ CCI.getTypeRange().getBegin(),</div><div>+ CCI.getTypeRange().getEnd(),</div>
<div>+ CCI.getTypeSourceInfo());</div></div><div><br></div><div>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.</div>
<div><br></div><div><div>@@ -719,7 +739,8 @@</div><div> /// CheckStaticCast - Check that a static_cast\<DestType\>(SrcExpr) is valid.</div><div> /// Refer to C++ 5.2.9 for details. Static casts are mostly used for making</div>
<div> /// implicit conversions explicit and getting rid of data loss warnings.</div><div><br></div><div>This comment should be updated to indicate that this function (sometimes) creates a CXXStaticCast expression.</div><div>
<br></div><div>-void CastOperation::CheckStaticCast() {</div><div>+void CastOperation::CheckStaticCast(bool &CastNodesCreated,</div><div>+ Sema::CheckedConversionInfo CCI) {</div></div>
<div><br></div><div>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.</div><div><br></div><div><div>+ CastNodesCreated = (SrcExpr.get() != SrcExprOrig &&</div>
<div>+ Kind != CK_ConstructorConversion);</div></div><div><br></div><div>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?</div>
<div><br></div><div>These last few comments also apply to the CXXCStyleCast case.</div><div><br></div><div><div>Index: lib/Sema/SemaExprCXX.cpp</div><div>===================================================================</div>
<div>--- lib/Sema/SemaExprCXX.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 151277)</div><div>+++ lib/Sema/SemaExprCXX.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div>
</div><div><div>@@ -2670,29 +2671,30 @@</div><div> </div><div> // _Complex x -> x</div><div> From = ImpCastExprToType(From, ElType,</div><div>- isFloatingComplex ? CK_FloatingComplexToReal</div>
<div>- : CK_IntegralComplexToReal, </div><div>- VK_RValue, /*BasePath=*/0, CCK).take();</div><div>+ isFloatingComplex ? CK_FloatingComplexToReal</div>
<div>+ : CK_IntegralComplexToReal).take();</div><div> </div><div> // x -> y</div><div> if (Context.hasSameUnqualifiedType(ElType, ToType)) {</div><div> // do nothing</div>
<div> } else if (ToType->isRealFloatingType()) {</div><div>- From = ImpCastExprToType(From, ToType,</div><div>- isFloatingComplex ? CK_FloatingCast : CK_IntegralToFloating, </div><div>- VK_RValue, /*BasePath=*/0, CCK).take();</div>
<div>+ From = CastExprToType(From, ToType,</div><div>+ isFloatingComplex ? CK_FloatingCast </div><div>+ : CK_IntegralToFloating, </div><div>
+ VK_RValue, /*BasePath=*/0, CCI).take();</div><div> } else {</div><div> assert(ToType->isIntegerType());</div><div>- From = ImpCastExprToType(From, ToType,</div><div>- isFloatingComplex ? CK_FloatingToIntegral : CK_IntegralCast, </div>
<div>- VK_RValue, /*BasePath=*/0, CCK).take();</div><div>+ From = CastExprToType(From, ToType,</div><div>+ isFloatingComplex ? CK_FloatingToIntegral</div>
<div>+ : CK_IntegralCast, </div><div>+ VK_RValue, /*BasePath=*/0, CCI).take();</div><div> }</div><div> }</div><div> break;</div></div>
<div><br></div><div>In the ElType == ToType case, it looks like this could still produce a no-op static cast containing an implicit cast.</div><div><br></div><div>Thanks!</div><div>Richard</div></div>