Page MenuHomePhabricator

[AST] Sink 'part of explicit cast' down into ImplicitCastExpr
ClosedPublic

Authored by lebedev.ri on Jul 26 2018, 1:35 AM.

Details

Summary

As discussed in IRC with @rsmith, it is slightly not good to keep that in the CastExpr itself:
Given the explicit cast, which is represented in AST as an ExplicitCastExpr + ImplicitCastExpr's,
only the ImplicitCastExpr's will be marked as PartOfExplicitCast, but not the ExplicitCastExpr itself.
Thus, it is only ever true for ImplicitCastExpr's, so we don't need to write/read/dump it for ExplicitCastExpr's.

We don't need to worry that we write the PartOfExplicitCast in PCH after CastExpr::path_iterator,
since the ExprImplicitCastAbbrev is only used when the NumBaseSpecs == 0, i.e. there is no 'path'.

Diff Detail

Repository
rC Clang

Event Timeline

lebedev.ri edited the summary of this revision. (Show Details)Jul 26 2018, 1:38 AM

2 small items, otherwise looks good.

include/clang/AST/Expr.h
2825

So, I'd prefer that this line get left in. Removing this makes it the single unused item in CastExprBitfields, so leaving it uninitialized is likely a bad idea.

lib/Sema/SemaCast.cpp
96–97

I think I'd prefer just using a different variable in the 'while' loop to avoid the cast. something like while((auto ICE =....

That said, either way this isn't a dyn_cast, this would be just a cast (since we KNOW the type).

2 small items, otherwise looks good.

Thank you for taking a look!

include/clang/AST/Expr.h
2825

Aha, ok.
That will result in less CastExprBits.PartOfExplicitCast = false; lines scattered across different constructors, too.

2833

Oh, i forget to init it here.

lib/Sema/SemaCast.cpp
96–97

I was trying to avoid having one extra variable, which may confuse things.
Let's see maybe it's not that ugly..

lebedev.ri marked 5 inline comments as done.

Address @erichkeane review notes.

lib/Sema/SemaCast.cpp
96–97

Indeed, cast<> should be used.
But trying to avoid this cast at all results in uglier code, so let's not.
(I need to call getSubExpr() on this new ImplicitCastExpr, so i'd need to maintain two variables, etc...)

erichkeane accepted this revision.Jul 26 2018, 7:39 AM
This revision is now accepted and ready to land.Jul 26 2018, 7:39 AM
rsmith added inline comments.Jul 26 2018, 1:21 PM
include/clang/AST/Expr.h
2929

Please also rename this to`isPartOfExplicitCast` as requested on IRC.

include/clang/AST/Stmt.h
206

representable for -> set for

lib/Sema/SemaCast.cpp
96–97

Maybe something like:

for (auto *Next = CE->getSubExpr(); auto *ICE = dyn_cast<ImplicitCastExpr>(Next); Next = ICE->getSubExpr())
  ICE->setIsPartOfExplicitCast(true);

or just

for (; auto *ICE = dyn_cast<ImplicitCastExpr>(CE->getSubExpr()); CE = ICE)
  ICE->setIsPartOfExplicitCast(true);
lebedev.ri marked 3 inline comments as done.

Address @rsmith review notes.

include/clang/AST/Expr.h
2929

Right, sorry.

lib/Sema/SemaCast.cpp
96–97

Oh wow, one can define variables not just in the "init-statement", how did i not notice this earlier?!
That changes the picture indeed.

rsmith accepted this revision.Jul 26 2018, 5:37 PM
This revision was automatically updated to reflect the committed changes.