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

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

Differential Revision: https://reviews.llvm.org/D57266

Reviewed By: aaron.ballman



git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@353006 91177308-0d34-0410-b5e6-96231b3b80d8

7 files changed, 94 insertions, 89 deletions

diff --git a/lib/ARCMigrate/TransRetainReleaseDealloc.cpp b/lib/ARCMigrate/TransRetainReleaseDealloc.cpp
index 347c198f68..b76fc65574 100644
--- a/lib/ARCMigrate/TransRetainReleaseDealloc.cpp
+++ b/lib/ARCMigrate/TransRetainReleaseDealloc.cpp
@@ -269,8 +269,8 @@ private:
 
     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)
@@ -280,8 +280,8 @@ private:
       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);
   }
diff --git a/lib/ARCMigrate/TransformActions.cpp b/lib/ARCMigrate/TransformActions.cpp
index aa1a2c50c5..f76cc2c130 100644
--- a/lib/ARCMigrate/TransformActions.cpp
+++ b/lib/ARCMigrate/TransformActions.cpp
@@ -313,7 +313,9 @@ 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);
 }
 
diff --git a/lib/ARCMigrate/Transforms.cpp b/lib/ARCMigrate/Transforms.cpp
index 6ca748bcdd..59b80a917e 100644
--- a/lib/ARCMigrate/Transforms.cpp
+++ b/lib/ARCMigrate/Transforms.cpp
@@ -286,10 +286,11 @@ private:
   void mark(Stmt *S) {
     if (!S) return;
 
-    while (LabelStmt *Label = dyn_cast<LabelStmt>(S))
+    while (auto *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 (auto *E = dyn_cast<Expr>(S))
       Removables.insert(E);
   }
 };
diff --git a/lib/AST/Expr.cpp b/lib/AST/Expr.cpp
index 2948acd3b5..6d3ad377a3 100644
--- a/lib/AST/Expr.cpp
+++ b/lib/AST/Expr.cpp
@@ -2556,32 +2556,66 @@ QualType Expr::findBoundMemberType(const Expr *expr) {
   return QualType();
 }
 
-Expr* Expr::IgnoreParens() {
-  Expr* E = this;
+Expr *Expr::IgnoreImpCasts() {
+  Expr *E = this;
   while (true) {
-    if (ParenExpr* P = dyn_cast<ParenExpr>(E)) {
-      E = P->getSubExpr();
+    if (auto *ICE = dyn_cast<ImplicitCastExpr>(E))
+      E = ICE->getSubExpr();
+    else if (auto *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 (auto *PE = dyn_cast<ParenExpr>(E)) {
+      E = PE->getSubExpr();
       continue;
     }
-    if (UnaryOperator* P = dyn_cast<UnaryOperator>(E)) {
-      if (P->getOpcode() == UO_Extension) {
-        E = P->getSubExpr();
+    if (auto *UO = dyn_cast<UnaryOperator>(E)) {
+      if (UO->getOpcode() == UO_Extension) {
+        E = UO->getSubExpr();
         continue;
       }
     }
-    if (GenericSelectionExpr* P = dyn_cast<GenericSelectionExpr>(E)) {
-      if (!P->isResultDependent()) {
-        E = P->getResultExpr();
+    if (auto *GSE = dyn_cast<GenericSelectionExpr>(E)) {
+      if (!GSE->isResultDependent()) {
+        E = GSE->getResultExpr();
         continue;
       }
     }
-    if (ChooseExpr* P = dyn_cast<ChooseExpr>(E)) {
-      if (!P->isConditionDependent()) {
-        E = P->getChosenSubExpr();
+    if (auto *CE = dyn_cast<ChooseExpr>(E)) {
+      if (!CE->isConditionDependent()) {
+        E = CE->getChosenSubExpr();
         continue;
       }
     }
-    if (ConstantExpr *CE = dyn_cast<ConstantExpr>(E)) {
+    if (auto *CE = dyn_cast<ConstantExpr>(E)) {
       E = CE->getSubExpr();
       continue;
     }
@@ -2595,21 +2629,19 @@ Expr *Expr::IgnoreParenCasts() {
   Expr *E = this;
   while (true) {
     E = E->IgnoreParens();
-    if (CastExpr *P = dyn_cast<CastExpr>(E)) {
-      E = P->getSubExpr();
+    if (auto *CE = dyn_cast<CastExpr>(E)) {
+      E = CE->getSubExpr();
       continue;
     }
-    if (MaterializeTemporaryExpr *Materialize
-                                      = dyn_cast<MaterializeTemporaryExpr>(E)) {
-      E = Materialize->GetTemporaryExpr();
+    if (auto *MTE = dyn_cast<MaterializeTemporaryExpr>(E)) {
+      E = MTE->GetTemporaryExpr();
       continue;
     }
-    if (SubstNonTypeTemplateParmExpr *NTTP
-                                  = dyn_cast<SubstNonTypeTemplateParmExpr>(E)) {
+    if (auto *NTTP = dyn_cast<SubstNonTypeTemplateParmExpr>(E)) {
       E = NTTP->getReplacement();
       continue;
     }
-    if (FullExpr *FE = dyn_cast<FullExpr>(E)) {
+    if (auto *FE = dyn_cast<FullExpr>(E)) {
       E = FE->getSubExpr();
       continue;
     }
@@ -2620,21 +2652,19 @@ Expr *Expr::IgnoreParenCasts() {
 Expr *Expr::IgnoreCasts() {
   Expr *E = this;
   while (true) {
-    if (CastExpr *P = dyn_cast<CastExpr>(E)) {
-      E = P->getSubExpr();
+    if (auto *CE = dyn_cast<CastExpr>(E)) {
+      E = CE->getSubExpr();
       continue;
     }
-    if (MaterializeTemporaryExpr *Materialize
-        = dyn_cast<MaterializeTemporaryExpr>(E)) {
-      E = Materialize->GetTemporaryExpr();
+    if (auto *MTE = dyn_cast<MaterializeTemporaryExpr>(E)) {
+      E = MTE->GetTemporaryExpr();
       continue;
     }
-    if (SubstNonTypeTemplateParmExpr *NTTP
-        = dyn_cast<SubstNonTypeTemplateParmExpr>(E)) {
+    if (auto *NTTP = dyn_cast<SubstNonTypeTemplateParmExpr>(E)) {
       E = NTTP->getReplacement();
       continue;
     }
-    if (FullExpr *FE = dyn_cast<FullExpr>(E)) {
+    if (auto *FE = dyn_cast<FullExpr>(E)) {
       E = FE->getSubExpr();
       continue;
     }
@@ -2650,20 +2680,18 @@ Expr *Expr::IgnoreParenLValueCasts() {
   Expr *E = this;
   while (true) {
     E = E->IgnoreParens();
-    if (CastExpr *P = dyn_cast<CastExpr>(E)) {
-      if (P->getCastKind() == CK_LValueToRValue) {
-        E = P->getSubExpr();
+    if (auto *CE = dyn_cast<CastExpr>(E)) {
+      if (CE->getCastKind() == CK_LValueToRValue) {
+        E = CE->getSubExpr();
         continue;
       }
-    } else if (MaterializeTemporaryExpr *Materialize
-                                      = dyn_cast<MaterializeTemporaryExpr>(E)) {
-      E = Materialize->GetTemporaryExpr();
+    } else if (auto *MTE = dyn_cast<MaterializeTemporaryExpr>(E)) {
+      E = MTE->GetTemporaryExpr();
       continue;
-    } else if (SubstNonTypeTemplateParmExpr *NTTP
-                                  = dyn_cast<SubstNonTypeTemplateParmExpr>(E)) {
+    } else if (auto *NTTP = dyn_cast<SubstNonTypeTemplateParmExpr>(E)) {
       E = NTTP->getReplacement();
       continue;
-    } else if (FullExpr *FE = dyn_cast<FullExpr>(E)) {
+    } else if (auto *FE = dyn_cast<FullExpr>(E)) {
       E = FE->getSubExpr();
       continue;
     }
@@ -2676,7 +2704,7 @@ Expr *Expr::ignoreParenBaseCasts() {
   Expr *E = this;
   while (true) {
     E = E->IgnoreParens();
-    if (CastExpr *CE = dyn_cast<CastExpr>(E)) {
+    if (auto *CE = dyn_cast<CastExpr>(E)) {
       if (CE->getCastKind() == CK_DerivedToBase ||
           CE->getCastKind() == CK_UncheckedDerivedToBase ||
           CE->getCastKind() == CK_NoOp) {
@@ -2693,17 +2721,15 @@ Expr *Expr::IgnoreParenImpCasts() {
   Expr *E = this;
   while (true) {
     E = E->IgnoreParens();
-    if (ImplicitCastExpr *P = dyn_cast<ImplicitCastExpr>(E)) {
-      E = P->getSubExpr();
+    if (auto *ICE = dyn_cast<ImplicitCastExpr>(E)) {
+      E = ICE->getSubExpr();
       continue;
     }
-    if (MaterializeTemporaryExpr *Materialize
-                                      = dyn_cast<MaterializeTemporaryExpr>(E)) {
-      E = Materialize->GetTemporaryExpr();
+    if (auto *MTE = dyn_cast<MaterializeTemporaryExpr>(E)) {
+      E = MTE->GetTemporaryExpr();
       continue;
     }
-    if (SubstNonTypeTemplateParmExpr *NTTP
-                                  = dyn_cast<SubstNonTypeTemplateParmExpr>(E)) {
+    if (auto *NTTP = dyn_cast<SubstNonTypeTemplateParmExpr>(E)) {
       E = NTTP->getReplacement();
       continue;
     }
@@ -2712,7 +2738,7 @@ Expr *Expr::IgnoreParenImpCasts() {
 }
 
 Expr *Expr::IgnoreConversionOperator() {
-  if (CXXMemberCallExpr *MCE = dyn_cast<CXXMemberCallExpr>(this)) {
+  if (auto *MCE = dyn_cast<CXXMemberCallExpr>(this)) {
     if (MCE->getMethodDecl() && isa<CXXConversionDecl>(MCE->getMethodDecl()))
       return MCE->getImplicitObjectArgument();
   }
@@ -2722,15 +2748,15 @@ Expr *Expr::IgnoreConversionOperator() {
 /// 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)) {
+    if (auto *CE = 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();
+      Expr *SE = CE->getSubExpr();
 
       if (Ctx.hasSameUnqualifiedType(E->getType(), SE->getType())) {
         E = SE;
@@ -2747,8 +2773,7 @@ Expr *Expr::IgnoreParenNoopCasts(ASTContext &Ctx) {
       }
     }
 
-    if (SubstNonTypeTemplateParmExpr *NTTP
-                                  = dyn_cast<SubstNonTypeTemplateParmExpr>(E)) {
+    if (auto *NTTP = dyn_cast<SubstNonTypeTemplateParmExpr>(E)) {
       E = NTTP->getReplacement();
       continue;
     }
diff --git a/lib/AST/Stmt.cpp b/lib/AST/Stmt.cpp
index e1a4dfc10e..d1a4dc074b 100644
--- a/lib/AST/Stmt.cpp
+++ b/lib/AST/Stmt.cpp
@@ -117,30 +117,6 @@ 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) {
diff --git a/lib/Analysis/ReachableCode.cpp b/lib/Analysis/ReachableCode.cpp
index 1e174e60d6..cc64efa7f0 100644
--- a/lib/Analysis/ReachableCode.cpp
+++ b/lib/Analysis/ReachableCode.cpp
@@ -192,9 +192,10 @@ static bool isConfigurationValue(const Stmt *S,
   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.
diff --git a/lib/Tooling/ASTDiff/ASTDiff.cpp b/lib/Tooling/ASTDiff/ASTDiff.cpp
index 0d456d1c8a..69eff20bff 100644
--- a/lib/Tooling/ASTDiff/ASTDiff.cpp
+++ b/lib/Tooling/ASTDiff/ASTDiff.cpp
@@ -237,8 +237,8 @@ struct PreorderVisitor : public RecursiveASTVisitor<PreorderVisitor> {
     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);