This is an archive of the discontinued LLVM Phabricator instance.

[AST] Update the comments of the various Expr::Ignore* + Related cleanups
ClosedPublic

Authored by riccibruno on Jan 25 2019, 4:29 PM.

Details

Summary

The description of what the various Expr::Ignore* do has drifted from the actual implementation.

Inspection reveals that IgnoreParenImpCasts() is not equivalent to doing IgnoreParens() + IgnoreImpCasts() until reaching a fixed point, but IgnoreParenCasts() is equivalent to doing IgnoreParens() + IgnoreCasts() until reaching a fixed point. There is also a fair amount of duplication in the various Expr::Ignore* functions which increase the chance of further future inconsistencies. In preparation for the next patch which will factor out the implementation of the various Expr::Ignore*, do the following cleanups:

Remove Stmt::IgnoreImplicit, in favor of Expr::IgnoreImplicit. IgnoreImplicit is the only function among all of the Expr::Ignore* which is available in Stmt. There are only a few users of Stmt::IgnoreImplicit. They can just use instead Expr::IgnoreImplicit like they have to do for the other Ignore*.

Move Expr::IgnoreImpCasts() from Expr.h to Expr.cpp. This made no difference in the run-time with my usual benchmark (-fsyntax-only on all of Boost).

While we are at it, make IgnoreParenNoopCasts take a const reference to the ASTContext for const correctness.

Update the comments to match what the Expr::Ignore* are actually doing. I am not sure that listing exactly what each Expr::Ignore* do is optimal, but it certainly looks better than the current state which is in my opinion between misleading and just plain wrong.

The whole patch is NFC (if you count removing Stmt::IgnoreImplicit as NFC).

Diff Detail

Repository
rL LLVM

Event Timeline

riccibruno created this revision.Jan 25 2019, 4:29 PM

Removed superfluous braces added in some if statements.

riccibruno edited the summary of this revision. (Show Details)Jan 26 2019, 5:24 AM
aaron.ballman accepted this revision.Jan 28 2019, 5:43 AM

LGTM with some nits. Thank you for the cleaned up comments, these are helpful!

include/clang/AST/Expr.h
749–750 ↗(On Diff #183695)

I'd remove the full stops from the list elements (same elsewhere).

848 ↗(On Diff #183695)

Want to fix up this name to match the coding conventions (in a separate patch, no review needed)?

lib/AST/Expr.cpp
2561 ↗(On Diff #183695)

I would appreciate some braces around the loop body -- I know they're not strictly required, but this is not a particularly simple body either.

2562 ↗(On Diff #183695)

auto * and same below.

This revision is now accepted and ready to land.Jan 28 2019, 5:43 AM
riccibruno marked 8 inline comments as done.Jan 28 2019, 5:51 AM
riccibruno added inline comments.
include/clang/AST/Expr.h
749–750 ↗(On Diff #183695)

Will do.

848 ↗(On Diff #183695)

Yeah I only see a few uses of ignoreParenBaseCasts so this should be easy to do.

lib/AST/Expr.cpp
2561 ↗(On Diff #183695)

Will do before landing. I am getting rid of all of these loops in the next patch anyway but I agree that braces are better here.

2562 ↗(On Diff #183695)

Yep.

riccibruno updated this revision to Diff 184945.Feb 3 2019, 6:05 AM
riccibruno marked 4 inline comments as done.

Addressed Aaron's comments.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2019, 11:53 AM