[cfe-commits] [cfe-dev] [review request] Removing redundant implicit casts in the AST, take 2
Richard Smith
richard at metafoo.co.uk
Thu Feb 23 18:28:57 PST 2012
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?
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120223/8918478d/attachment.html>
More information about the cfe-commits
mailing list