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

[riccibruno]

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).

[riccibruno]

Removed superfluous braces added in some if statements.

[aaron.ballman]

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

include/clang/AST/Expr.h

749–750

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

848–849

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

lib/AST/Expr.cpp

2561

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

auto * and same below.

[riccibruno]

include/clang/AST/Expr.h

749–750

Will do.

848–849

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

lib/AST/Expr.cpp

2561

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

Yep.

[riccibruno]

Addressed Aaron's comments.

include/clang/AST/Expr.h

public:
   /// Returns true if this expression is a bound member function.
   bool isBoundMemberFunction(ASTContext &Ctx) const;
 
   /// Given an expression of bound-member type, find the type
   /// of the member.  Returns null if this is an *overloaded* bound
   /// member expression.
   static QualType findBoundMemberType(const Expr *expr);
 
-  /// IgnoreImpCasts - Skip past any implicit casts which might
-  /// surround this expression.  Only skips ImplicitCastExprs.
+  /// Skip past any implicit casts which might surround this expression until
+  /// reaching a fixed point. Skips:
+  /// * ImplicitCastExpr.
+  /// * FullExpr.
   Expr *IgnoreImpCasts() LLVM_READONLY;
+  const Expr *IgnoreImpCasts() const {
+    return const_cast<Expr *>(this)->IgnoreImpCasts();
+  }
 
-  /// IgnoreImplicit - Skip past any implicit AST nodes which might
-  /// surround this expression.
-  Expr *IgnoreImplicit() LLVM_READONLY {
-    return cast<Expr>(Stmt::IgnoreImplicit());
+  /// Skip past any casts which might surround this expression until reaching
+  /// a fixed point. Skips:
+  /// * CastExpr.
+  /// * FullExpr.
+  /// * MaterializeTemporaryExpr.
+  /// * SubstNonTypeTemplateParmExpr.
+  Expr *IgnoreCasts() LLVM_READONLY;
+  const Expr *IgnoreCasts() const {
+    return const_cast<Expr *>(this)->IgnoreCasts();
   }
 
-  const Expr *IgnoreImplicit() const LLVM_READONLY {
-    return const_cast<Expr*>(this)->IgnoreImplicit();
+  /// Skip past any implicit AST nodes which might surround this expression
+  /// until reaching a fixed point. Skips:
+  /// * What IgnoreImpCasts() skips.
+  /// * MaterializeTemporaryExpr.
+  /// * CXXBindTemporaryExpr.
+  Expr *IgnoreImplicit() LLVM_READONLY;
+  const Expr *IgnoreImplicit() const {
+    return const_cast<Expr *>(this)->IgnoreImplicit();
   }
 
-  /// IgnoreParens - Ignore parentheses.  If this Expr is a ParenExpr, return
-  ///  its subexpression.  If that subexpression is also a ParenExpr,
-  ///  then this method recursively returns its subexpression, and so forth.
-  ///  Otherwise, the method returns the current Expr.
+  /// Skip past any parentheses which might surround this expression until
+  /// reaching a fixed point. Skips:
+  /// * ParenExpr.
+  /// * UnaryOperator if `UO_Extension`.
+  /// * GenericSelectionExpr if `!isResultDependent()`.
+  /// * ChooseExpr if `!isConditionDependent()`.
+  /// * ConstantExpr.
   Expr *IgnoreParens() LLVM_READONLY;
+  const Expr *IgnoreParens() const {
+    return const_cast<Expr *>(this)->IgnoreParens();
+  }
 
-  /// IgnoreParenCasts - Ignore parentheses and casts.  Strip off any ParenExpr
-  /// or CastExprs, returning their operand.
-  Expr *IgnoreParenCasts() LLVM_READONLY;
-
-  /// Ignore casts.  Strip off any CastExprs, returning their operand.
-  Expr *IgnoreCasts() LLVM_READONLY;
-
-  /// IgnoreParenImpCasts - Ignore parentheses and implicit casts.  Strip off
-  /// any ParenExpr or ImplicitCastExprs, returning their operand.
+  /// Skip past any parentheses and implicit casts which might surround this
+  /// expression until reaching a fixed point.
+  /// FIXME: IgnoreParenImpCasts really ought to be equivalent to
+  /// IgnoreParens() + IgnoreImpCasts() until reaching a fixed point. However
+  /// this is currently not the case. Instead IgnoreParenImpCasts() skips:
+  /// * What IgnoreParens() skips.
+  /// * ImplicitCastExpr.
+  /// * MaterializeTemporaryExpr.
+  /// * SubstNonTypeTemplateParmExpr.
   Expr *IgnoreParenImpCasts() LLVM_READONLY;
+  const Expr *IgnoreParenImpCasts() const {
+    return const_cast<Expr *>(this)->IgnoreParenImpCasts();
+  }
 
-  /// IgnoreConversionOperator - Ignore conversion operator. If this Expr is a
-  /// call to a conversion operator, return the argument.
-  Expr *IgnoreConversionOperator() LLVM_READONLY;
-
-  const Expr *IgnoreConversionOperator() const LLVM_READONLY {
-    return const_cast<Expr*>(this)->IgnoreConversionOperator();
+  /// Skip past any parentheses and casts which might surround this expression
+  /// until reaching a fixed point. Skips:
+  /// * What IgnoreParens() skips.
+  /// * What IgnoreCasts() skips.
+  Expr *IgnoreParenCasts() LLVM_READONLY;
+  const Expr *IgnoreParenCasts() const {
+    return const_cast<Expr *>(this)->IgnoreParenCasts();
   }
 
-  const Expr *IgnoreParenImpCasts() const LLVM_READONLY {
-    return const_cast<Expr*>(this)->IgnoreParenImpCasts();
+  /// Skip conversion operators. If this Expr is a call to a conversion
+  /// operator, return the argument.
+  Expr *IgnoreConversionOperator() LLVM_READONLY;
+  const Expr *IgnoreConversionOperator() const {
+    return const_cast<Expr *>(this)->IgnoreConversionOperator();
   }
 
-  /// Ignore parentheses and lvalue casts.  Strip off any ParenExpr and
-  /// CastExprs that represent lvalue casts, returning their operand.
+  /// Skip past any parentheses and lvalue casts which might surround this
+  /// expression until reaching a fixed point. Skips:
+  /// * What IgnoreParens() skips.
+  /// * What IgnoreCasts() skips, except that only lvalue-to-rvalue
+  ///   casts are skipped.
+  /// FIXME: This is intended purely as a temporary workaround for code
+  /// that hasn't yet been rewritten to do the right thing about those
+  /// casts, and may disappear along with the last internal use.
   Expr *IgnoreParenLValueCasts() LLVM_READONLY;
-
-  const Expr *IgnoreParenLValueCasts() const LLVM_READONLY {
-    return const_cast<Expr*>(this)->IgnoreParenLValueCasts();
+  const Expr *IgnoreParenLValueCasts() const {
+    return const_cast<Expr *>(this)->IgnoreParenLValueCasts();
   }
 
-  /// IgnoreParenNoopCasts - Ignore parentheses and casts that do not change the
-  /// value (including ptr->int casts of the same size).  Strip off any
-  /// ParenExpr or CastExprs, returning their operand.
-  Expr *IgnoreParenNoopCasts(ASTContext &Ctx) LLVM_READONLY;
+  /// Skip past any parenthese and casts which do not change the value
+  /// (including ptr->int casts of the same size) until reaching a fixed point.
+  /// Skips:
+  /// * What IgnoreParens() skips.
+  /// * CastExpr which do not change the value.
+  /// * SubstNonTypeTemplateParmExpr.
+  Expr *IgnoreParenNoopCasts(const ASTContext &Ctx) LLVM_READONLY;
+  const Expr *IgnoreParenNoopCasts(const ASTContext &Ctx) const {
+    return const_cast<Expr *>(this)->IgnoreParenNoopCasts(Ctx);
+  }
 
-  /// Ignore parentheses and derived-to-base casts.
+  /// Skip past any parentheses and derived-to-base casts until reaching a
+  /// fixed point. Skips:
+  /// * What IgnoreParens() skips.
+  /// * CastExpr which represent a derived-to-base cast (CK_DerivedToBase,
+  ///   CK_UncheckedDerivedToBase and CK_NoOp).
   Expr *ignoreParenBaseCasts() LLVM_READONLY;
-
-  const Expr *ignoreParenBaseCasts() const LLVM_READONLY {
-    return const_cast<Expr*>(this)->ignoreParenBaseCasts();
+  const Expr *ignoreParenBaseCasts() const {
+    return const_cast<Expr *>(this)->ignoreParenBaseCasts();
   }
 
   /// Determine whether this expression is a default function argument.
   ///
   /// Default arguments are implicitly generated in the abstract syntax tree
   /// by semantic analysis for function calls, object constructions, etc. in
   /// C++. Default arguments are represented by \c CXXDefaultArgExpr nodes;
   /// this routine also looks through any implicit casts to determine whether
   /// the expression is a default argument.
   bool isDefaultArgument() const;
 
   /// Determine whether the result of this expression is a
   /// temporary object of the given class type.
   bool isTemporaryObject(ASTContext &Ctx, const CXXRecordDecl *TempTy) const;
 
   /// Whether this expression is an implicit reference to 'this' in C++.
   bool isImplicitCXXThis() const;
 
-  const Expr *IgnoreImpCasts() const LLVM_READONLY {
-    return const_cast<Expr*>(this)->IgnoreImpCasts();
-  }
-  const Expr *IgnoreParens() const LLVM_READONLY {
-    return const_cast<Expr*>(this)->IgnoreParens();
-  }
-  const Expr *IgnoreParenCasts() const LLVM_READONLY {
-    return const_cast<Expr*>(this)->IgnoreParenCasts();
-  }
-  /// Strip off casts, but keep parentheses.
-  const Expr *IgnoreCasts() const LLVM_READONLY {
-    return const_cast<Expr*>(this)->IgnoreCasts();
-  }
-
-  const Expr *IgnoreParenNoopCasts(ASTContext &Ctx) const LLVM_READONLY {
-    return const_cast<Expr*>(this)->IgnoreParenNoopCasts(Ctx);
-  }
-
   static bool hasAnyTypeDependentArguments(ArrayRef<Expr *> Exprs);
 
   /// For an expression of class type or pointer to class type,
   /// return the most derived class decl the expression is known to refer to.
   ///
   /// If this expression is a cast, this method looks through it to find the
   /// most derived decl that can be inferred from the expression.
   /// This is valid because derived-to-base conversions have undefined
public:
   static bool classof(const Stmt *T) {
     return T->getStmtClass() == ImplicitCastExprClass;
   }
 
   friend TrailingObjects;
   friend class CastExpr;
 };
 
-inline Expr *Expr::IgnoreImpCasts() {
-  Expr *e = this;
-  while (true)
-    if (ImplicitCastExpr *ice = dyn_cast<ImplicitCastExpr>(e))
-      e = ice->getSubExpr();
-    else if (FullExpr *fe = dyn_cast<FullExpr>(e))
-      e = fe->getSubExpr();
-    else
-      break;
-  return e;
-}
-
 /// ExplicitCastExpr - An explicit cast written in the source
 /// code.
 ///
 /// This class is effectively an abstract class, because it provides
 /// the basic representation of an explicitly-written cast without
 /// specifying which kind of cast (C cast, functional cast, static
 /// cast, etc.) was written; specific derived classes represent the
 /// particular style of cast and its location information.

include/clang/AST/Stmt.h

void printPretty(raw_ostream &OS, PrinterHelper *Helper,
                    const PrintingPolicy &Policy, unsigned Indentation = 0,
                    StringRef NewlineSymbol = "\n",
                    const ASTContext *Context = nullptr) const;
 
   /// viewAST - Visualize an AST rooted at this Stmt* using GraphViz.  Only
   ///   works on systems with GraphViz (Mac OS X) or dot+gv installed.
   void viewAST() const;
 
-  /// Skip past any implicit AST nodes which might surround this
-  /// statement, such as ExprWithCleanups or ImplicitCastExpr nodes.
-  Stmt *IgnoreImplicit();
-  const Stmt *IgnoreImplicit() const {
-    return const_cast<Stmt *>(this)->IgnoreImplicit();
-  }
-
   /// Skip no-op (attributed, compound) container stmts and skip captured
   /// stmt at the top, if \a IgnoreCaptured is true.
   Stmt *IgnoreContainers(bool IgnoreCaptured = false);
   const Stmt *IgnoreContainers(bool IgnoreCaptured = false) const {
     return const_cast<Stmt *>(this)->IgnoreContainers(IgnoreCaptured);
   }
 
   const Stmt *stripLabelLikeStatements() const;

lib/ARCMigrate/TransRetainReleaseDealloc.cpp

std::pair<Stmt *, Stmt *> getPreviousAndNextStmt(Expr *E) {
     for (; currChildS != childE; ++currChildS) {
       if (*currChildS == InnerS)
         break;
       prevChildS = currChildS;
     }
 
     if (prevChildS != childE) {
       prevStmt = *prevChildS;
-      if (prevStmt)
-        prevStmt = prevStmt->IgnoreImplicit();
+      if (auto *E = dyn_cast_or_null<Expr>(prevStmt))
+        prevStmt = E->IgnoreImplicit();
     }
 
     if (currChildS == childE)
       return std::make_pair(prevStmt, nextStmt);
     ++currChildS;
     if (currChildS == childE)
       return std::make_pair(prevStmt, nextStmt);
 
     nextStmt = *currChildS;
-    if (nextStmt)
-      nextStmt = nextStmt->IgnoreImplicit();
+    if (auto *E = dyn_cast_or_null<Expr>(nextStmt))
+      nextStmt = E->IgnoreImplicit();
 
     return std::make_pair(prevStmt, nextStmt);
   }
 
   Decl *getReferencedDecl(Expr *E) {
     if (!E)
       return nullptr;
 

lib/ARCMigrate/TransformActions.cpp

void TransformActionsImpl::remove(SourceRange range) {
   data.R1 = range;
   CachedActions.push_back(data);
 }
 
 void TransformActionsImpl::removeStmt(Stmt *S) {
   assert(IsInTransaction && "Actions only allowed during a transaction");
   ActionData data;
   data.Kind = Act_RemoveStmt;
-  data.S = S->IgnoreImplicit(); // important for uniquing
+  if (auto *E = dyn_cast<Expr>(S))
+    S = E->IgnoreImplicit(); // important for uniquing
+  data.S = S;
   CachedActions.push_back(data);
 }
 
 void TransformActionsImpl::replace(SourceRange range, StringRef text) {
   assert(IsInTransaction && "Actions only allowed during a transaction");
   text = getUniqueText(text);
   remove(range);
   insert(range.getBegin(), text);

lib/ARCMigrate/Transforms.cpp

public:
   }
 
 private:
   void mark(Stmt *S) {
     if (!S) return;
 
     while (LabelStmt *Label = dyn_cast<LabelStmt>(S))
       S = Label->getSubStmt();
-    S = S->IgnoreImplicit();
-    if (Expr *E = dyn_cast<Expr>(S))
+    if (auto *E = dyn_cast<Expr>(S))
+      S = E->IgnoreImplicit();
+    if (Expr *E = dyn_cast<Expr>(S)) {
       Removables.insert(E);
     }
+  }
 };
 
 } // end anonymous namespace
 
 void trans::clearRefsIn(Stmt *S, ExprSet &refs) {
   ReferenceClear(refs).TraverseStmt(S);
 }
 

lib/AST/Expr.cpp

if (const BinaryOperator *op = dyn_cast<BinaryOperator>(expr)) {
     assert(type->isFunctionType());
     return type;
   }
 
   assert(isa<UnresolvedMemberExpr>(expr) || isa<CXXPseudoDestructorExpr>(expr));
   return QualType();
 }
 
+Expr *Expr::IgnoreImpCasts() {
+  Expr *E = this;
+  while (true)
+    if (ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(E))
+      E = ICE->getSubExpr();
+    else if (FullExpr *FE = dyn_cast<FullExpr>(E))
+      E = FE->getSubExpr();
+    else
+      break;
+  return E;
+}
+
+Expr *Expr::IgnoreImplicit() {
+  Expr *E = this;
+  Expr *LastE = nullptr;
+  while (E != LastE) {
+    LastE = E;
+
+    if (auto *ICE = dyn_cast<ImplicitCastExpr>(E))
+      E = ICE->getSubExpr();
+
+    if (auto *FE = dyn_cast<FullExpr>(E))
+      E = FE->getSubExpr();
+
+    if (auto *MTE = dyn_cast<MaterializeTemporaryExpr>(E))
+      E = MTE->GetTemporaryExpr();
+
+    if (auto *BTE = dyn_cast<CXXBindTemporaryExpr>(E))
+      E = BTE->getSubExpr();
+  }
+  return E;
+}
+
 Expr* Expr::IgnoreParens() {
   Expr* E = this;
   while (true) {
     if (ParenExpr* P = dyn_cast<ParenExpr>(E)) {
       E = P->getSubExpr();
       continue;
     }
     if (UnaryOperator* P = dyn_cast<UnaryOperator>(E)) {
if (MCE->getMethodDecl() && isa<CXXConversionDecl>(MCE->getMethodDecl()))
       return MCE->getImplicitObjectArgument();
   }
   return this;
 }
 
 /// IgnoreParenNoopCasts - Ignore parentheses and casts that do not change the
 /// value (including ptr->int casts of the same size).  Strip off any
 /// ParenExpr or CastExprs, returning their operand.
-Expr *Expr::IgnoreParenNoopCasts(ASTContext &Ctx) {
+Expr *Expr::IgnoreParenNoopCasts(const ASTContext &Ctx) {
   Expr *E = this;
   while (true) {
     E = E->IgnoreParens();
 
     if (CastExpr *P = dyn_cast<CastExpr>(E)) {
       // We ignore integer <-> casts that are of the same width, ptr<->ptr and
       // ptr<->int casts of the same width.  We also ignore all identity casts.
       Expr *SE = P->getSubExpr();

lib/AST/Stmt.cpp

void Stmt::addStmtClass(StmtClass s) {
   ++getStmtInfoTableEntry(s).Counter;
 }
 
 bool Stmt::StatisticsEnabled = false;
 void Stmt::EnableStatistics() {
   StatisticsEnabled = true;
 }
 
-Stmt *Stmt::IgnoreImplicit() {
-  Stmt *s = this;
-
-  Stmt *lasts = nullptr;
-
-  while (s != lasts) {
-    lasts = s;
-
-    if (auto *fe = dyn_cast<FullExpr>(s))
-      s = fe->getSubExpr();
-
-    if (auto *mte = dyn_cast<MaterializeTemporaryExpr>(s))
-      s = mte->GetTemporaryExpr();
-
-    if (auto *bte = dyn_cast<CXXBindTemporaryExpr>(s))
-      s = bte->getSubExpr();
-
-    if (auto *ice = dyn_cast<ImplicitCastExpr>(s))
-      s = ice->getSubExpr();
-  }
-
-  return s;
-}
-
 /// Skip no-op (attributed, compound) container stmts and skip captured
 /// stmt at the top, if \a IgnoreCaptured is true.
 Stmt *Stmt::IgnoreContainers(bool IgnoreCaptured) {
   Stmt *S = this;
   if (IgnoreCaptured)
     if (auto CapS = dyn_cast_or_null<CapturedStmt>(S))
       S = CapS->getCapturedStmt();
   while (true) {

lib/Analysis/ReachableCode.cpp

 static bool isConfigurationValue(const Stmt *S,
                                  Preprocessor &PP,
                                  SourceRange *SilenceableCondVal = nullptr,
                                  bool IncludeIntegers = true,
                                  bool WrappedInParens = false) {
   if (!S)
     return false;
 
-  S = S->IgnoreImplicit();
+  if (const auto *Ex = dyn_cast<Expr>(S))
+    S = Ex->IgnoreImplicit();
 
-  if (const Expr *Ex = dyn_cast<Expr>(S))
+  if (const auto *Ex = dyn_cast<Expr>(S)) {
     S = Ex->IgnoreCasts();
+  }
 
   // Special case looking for the sigil '()' around an integer literal.
   if (const ParenExpr *PE = dyn_cast<ParenExpr>(S))
     if (!PE->getBeginLoc().isMacroID())
       return isConfigurationValue(PE->getSubExpr(), PP, SilenceableCondVal,
                                   IncludeIntegers, true);
 
   if (const Expr *Ex = dyn_cast<Expr>(S))

lib/Tooling/ASTDiff/ASTDiff.cpp

bool TraverseDecl(Decl *D) {
     if (isNodeExcluded(Tree.AST.getSourceManager(), D))
       return true;
     auto SavedState = PreTraverse(D);
     RecursiveASTVisitor<PreorderVisitor>::TraverseDecl(D);
     PostTraverse(SavedState);
     return true;
   }
   bool TraverseStmt(Stmt *S) {
-    if (S)
-      S = S->IgnoreImplicit();
+    if (auto *E = dyn_cast_or_null<Expr>(S))
+      S = E->IgnoreImplicit();
     if (isNodeExcluded(Tree.AST.getSourceManager(), S))
       return true;
     auto SavedState = PreTraverse(S);
     RecursiveASTVisitor<PreorderVisitor>::TraverseStmt(S);
     PostTraverse(SavedState);
     return true;
   }
   bool TraverseType(QualType T) { return true; }