Add the ability to use the children() range API in a const-correct manner

[aaron.ballman]

Summary

We already have children() but not children() const, which is causing some problems for me on an out of tree project. Most of the changes here are mechanical, except for the work done to ConstStmtIterator, which cheats and uses a const_cast. Since none of these are *actually* const, this should not trigger UB.

[aaron.ballman]

Missed a case for UnaryExprOrTypeTraitExpr.

[aaron.ballman]

Ping.

Because I expect this to be fairly uncontroversial, I'll give it a week in case folks have comments, then commit (and will handle anything in post-commit review).

[dblaikie]

As I mentioned to Craig Topper recently on another review, generally when implementing const and non-const overloads the non-const is implemented in terms of the const overload (& const_casts away const on the result). This ensures no UB if the const overload is called on a truly const object. Even if there aren't any truly const Stmts today, I'd still prefer the code be written so as not to assume they couldn't exist.

[aaron.ballman]

In D31153#711287, @dblaikie wrote:

As I mentioned to Craig Topper recently on another review, generally when implementing const and non-const overloads the non-const is implemented in terms of the const overload (& const_casts away const on the result). This ensures no UB if the const overload is called on a truly const object. Even if there aren't any truly const Stmts today, I'd still prefer the code be written so as not to assume they couldn't exist.

In practice, this is really hard to do because there's no such thing as a const_iterator_cast operation to convert a const_iterator into an iterator, unless you have the underlying container handy. Since many of these uses do not have an underlying container (they're often trailing objects), I'm not certain this is practical (though I agree with the reasoning). Suggestions welcome, however.

[aaron.ballman]

Addressing David's review feedback.

[dblaikie]

include/clang/AST/Expr.h

4025

If this is adding const, can you use a weaker cast? (I guess what would be ideal here would be implicit_cast - if you're interested you could add that - otherwise... yeah, I guess static_cast could do other nasty things, etc, so there's nothing good)

include/clang/AST/StmtIterator.h

148–150

Maybe I'm missing something - what stops code from doing this conversion implicitly/accidentally all over the place? (I would've expecetd cast_away_const to be a friend or something, to only allow the conversion through this explicit call/operation)

[aaron.ballman]

Addressing review comments.

include/clang/AST/Expr.h

4025

I'm not certain what you mean. This is adding the const qualifier so that we call the proper overload. I'm not certain of what a weaker cast would be.

include/clang/AST/StmtIterator.h

148–150

Good catch, I've corrected this.

[dblaikie]

Looks good to me - thanks!

include/clang/AST/StmtIterator.h

137

the "struct" here is a bit atypical/should be removed

[aaron.ballman]

include/clang/AST/StmtIterator.h

137

The forward reference is needed because ConstStmtIterator is defined below StmtIterator. If I use class rather than struct, then MSVC diagnoses because of the mismatched class-key (which is a pretty low-value diagnostic, but I see no reason to disable it either).

[aaron.ballman]

Commit in r299981.

include/clang/AST/Expr.h

     return child_range(child_iterator(), child_iterator());
   }
 
+  const_child_range children() const {
+    return const_child_range(const_child_iterator(), const_child_iterator());
+  }
+
   /// The source expression of an opaque value expression is the
   /// expression which originally generated the value.  This is
   /// provided as a convenience for analyses that don't wish to

     return child_range(child_iterator(), child_iterator());
   }
 
+  const_child_range children() const {
+    return const_child_range(const_child_iterator(), const_child_iterator());
+  }
+
   friend TrailingObjects;
   friend class ASTStmtReader;
   friend class ASTStmtWriter;

 
   // Iterators
   child_range children() { return child_range(&FnName, &FnName + 1); }
+  const_child_range children() const {
+    return const_child_range(&FnName, &FnName + 1);
+  }
 
   friend class ASTStmtReader;
 };

   child_range children() {
     return child_range(child_iterator(), child_iterator());
   }
+  const_child_range children() const {
+    return const_child_range(const_child_iterator(), const_child_iterator());
+  }
 };
 
 class CharacterLiteral : public Expr {

   child_range children() {
     return child_range(child_iterator(), child_iterator());
   }
+  const_child_range children() const {
+    return const_child_range(const_child_iterator(), const_child_iterator());
+  }
 };
 
 class FloatingLiteral : public Expr, private APFloatStorage {

   child_range children() {
     return child_range(child_iterator(), child_iterator());
   }
+  const_child_range children() const {
+    return const_child_range(const_child_iterator(), const_child_iterator());
+  }
 };
 
 /// ImaginaryLiteral - We support imaginary integer and floating point literals,

 
   // Iterators
   child_range children() { return child_range(&Val, &Val+1); }
+  const_child_range children() const {
+    return const_child_range(&Val, &Val + 1);
+  }
 };
 
 /// StringLiteral - This represents a string literal expression, e.g. "foo"

   child_range children() {
     return child_range(child_iterator(), child_iterator());
   }
+  const_child_range children() const {
+    return const_child_range(const_child_iterator(), const_child_iterator());
+  }
 };
 
 /// ParenExpr - This represents a parethesized expression, e.g. "(1)".  This

 
   // Iterators
   child_range children() { return child_range(&Val, &Val+1); }
+  const_child_range children() const {
+    return const_child_range(&Val, &Val + 1);
+  }
 };
 
 /// UnaryOperator - This represents the unary-expression's (except sizeof and

 
   // Iterators
   child_range children() { return child_range(&Val, &Val+1); }
+  const_child_range children() const {
+    return const_child_range(&Val, &Val + 1);
+  }
 };
 
 /// Helper class for OffsetOfExpr.

     Stmt **begin = reinterpret_cast<Stmt **>(getTrailingObjects<Expr *>());
     return child_range(begin, begin + NumExprs);
   }
+  const_child_range children() const {
+    Stmt *const *begin =
+        reinterpret_cast<Stmt *const *>(getTrailingObjects<Expr *>());
+    return const_child_range(begin, begin + NumExprs);
+  }
   friend TrailingObjects;
 };
 

 
   // Iterators
   child_range children();
+  const_child_range children() const;
 };
 
 //===----------------------------------------------------------------------===//

   child_range children() {
     return child_range(&SubExprs[0], &SubExprs[0]+END_EXPR);
   }
+  const_child_range children() const {
+    return const_child_range(&SubExprs[0], &SubExprs[0] + END_EXPR);
+  }
 };
 
 /// CallExpr - Represents a function call (C99 6.5.2.2, C++ [expr.call]).

     return child_range(&SubExprs[0],
                        &SubExprs[0]+NumArgs+getNumPreArgs()+PREARGS_START);
   }
+
+  const_child_range children() const {
+    return const_child_range(&SubExprs[0], &SubExprs[0] + NumArgs +
+                                               getNumPreArgs() + PREARGS_START);
+  }
 };
 
 /// Extra data stored in some MemberExpr objects.

 
   // Iterators
   child_range children() { return child_range(&Base, &Base+1); }
+  const_child_range children() const {
+    return const_child_range(&Base, &Base + 1);
+  }
 
   friend TrailingObjects;
   friend class ASTReader;

 
   // Iterators
   child_range children() { return child_range(&Init, &Init+1); }
+  const_child_range children() const {
+    return const_child_range(&Init, &Init + 1);
+  }
 };
 
 /// CastExpr - Base class for type casts, including both implicit

 
   // Iterators
   child_range children() { return child_range(&Op, &Op+1); }
+  const_child_range children() const { return const_child_range(&Op, &Op + 1); }
 };
 
 /// ImplicitCastExpr - Allows us to explicitly represent implicit type

   child_range children() {
     return child_range(&SubExprs[0], &SubExprs[0]+END_EXPR);
   }
+  const_child_range children() const {
+    return const_child_range(&SubExprs[0], &SubExprs[0] + END_EXPR);
+  }
 
   // Set the FP contractability status of this operator. Only meaningful for
   // operations on floating point types.

   child_range children() {
     return child_range(&SubExprs[0], &SubExprs[0]+END_EXPR);
   }
+  const_child_range children() const {
+    return const_child_range(&SubExprs[0], &SubExprs[0] + END_EXPR);
+  }
 };
 
 /// BinaryConditionalOperator - The GNU extension to the conditional

   child_range children() {
     return child_range(SubExprs, SubExprs + NUM_SUBEXPRS);
   }
+  const_child_range children() const {
+    return const_child_range(SubExprs, SubExprs + NUM_SUBEXPRS);
+  }
 };
 
 inline Expr *AbstractConditionalOperator::getCond() const {

   child_range children() {
     return child_range(child_iterator(), child_iterator());
   }
+  const_child_range children() const {
+    return const_child_range(const_child_iterator(), const_child_iterator());
+  }
 };
 
 /// StmtExpr - This is the GNU Statement Expression extension: ({int X=4; X;}).

 
   // Iterators
   child_range children() { return child_range(&SubStmt, &SubStmt+1); }
+  const_child_range children() const {
+    return const_child_range(&SubStmt, &SubStmt + 1);
+  }
 };
 
 /// ShuffleVectorExpr - clang-specific builtin-in function

   child_range children() {
     return child_range(&SubExprs[0], &SubExprs[0]+NumExprs);
   }
+  const_child_range children() const {
+    return const_child_range(&SubExprs[0], &SubExprs[0] + NumExprs);
+  }
 };
 
 /// ConvertVectorExpr - Clang builtin function __builtin_convertvector

 
   // Iterators
   child_range children() { return child_range(&SrcExpr, &SrcExpr+1); }
+  const_child_range children() const {
+    return const_child_range(&SrcExpr, &SrcExpr + 1);
+  }
 };
 
 /// ChooseExpr - GNU builtin-in function __builtin_choose_expr.

   child_range children() {
     return child_range(&SubExprs[0], &SubExprs[0]+END_EXPR);
   }
+  const_child_range children() const {
+    return const_child_range(&SubExprs[0], &SubExprs[0] + END_EXPR);
+  }
 };
 
 /// GNUNullExpr - Implements the GNU __null extension, which is a name

   child_range children() {
     return child_range(child_iterator(), child_iterator());
   }
+  const_child_range children() const {
+    return const_child_range(const_child_iterator(), const_child_iterator());
+  }
 };
 
 /// Represents a call to the builtin function \c __builtin_va_arg.

 
   // Iterators
   child_range children() { return child_range(&Val, &Val+1); }
+  const_child_range children() const {
+    return const_child_range(&Val, &Val + 1);
+  }
 };
 
 /// @brief Describes an C or C++ initializer list.

 
   // Iterators
   child_range children() {
+    const_child_range CCR = const_cast<const InitListExpr *>(this)->children();
+    return child_range(cast_away_const(CCR.begin()),
+                       cast_away_const(CCR.end()));
+  }
+
+  const_child_range children() const {
     // FIXME: This does not include the array filler expression.
     if (InitExprs.empty())
-      return child_range(child_iterator(), child_iterator());
-    return child_range(&InitExprs[0], &InitExprs[0] + InitExprs.size());
+      return const_child_range(const_child_iterator(), const_child_iterator());
+    return const_child_range(&InitExprs[0], &InitExprs[0] + InitExprs.size());
   }
 
   typedef InitExprsTy::iterator iterator;

     Stmt **begin = getTrailingObjects<Stmt *>();
     return child_range(begin, begin + NumSubExprs);
   }
+  const_child_range children() const {
+    Stmt * const *begin = getTrailingObjects<Stmt *>();
+    return const_child_range(begin, begin + NumSubExprs);
+  }
 
   friend TrailingObjects;
 };

   child_range children() {
     return child_range(child_iterator(), child_iterator());
   }
+  const_child_range children() const {
+    return const_child_range(const_child_iterator(), const_child_iterator());
+  }
 };
 
 // In cases like:

   child_range children() {
     return child_range(&BaseAndUpdaterExprs[0], &BaseAndUpdaterExprs[0] + 2);
   }
+  const_child_range children() const {
+    return const_child_range(&BaseAndUpdaterExprs[0],
+                             &BaseAndUpdaterExprs[0] + 2);
+  }
 };
 
 /// \brief Represents a loop initializing the elements of an array.

   child_range children() {
     return child_range(SubExprs, SubExprs + 2);
   }
+  const_child_range children() const {
+    return const_child_range(SubExprs, SubExprs + 2);
+  }
 
   friend class ASTReader;
   friend class ASTStmtReader;

   child_range children() {
     return child_range(child_iterator(), child_iterator());
   }
+  const_child_range children() const {
+    return const_child_range(const_child_iterator(), const_child_iterator());
+  }
 
   friend class ASTReader;
   friend class ASTStmtReader;

   child_range children() {
     return child_range(child_iterator(), child_iterator());
   }
+  const_child_range children() const {
+    return const_child_range(const_child_iterator(), const_child_iterator());
+  }
 };
 
 class ParenListExpr : public Expr {

   child_range children() {
     return child_range(&Exprs[0], &Exprs[0]+NumExprs);
   }
+  const_child_range children() const {
+    return const_child_range(&Exprs[0], &Exprs[0] + NumExprs);
+  }
 
   friend class ASTStmtReader;
   friend class ASTStmtWriter;

   child_range children() {
     return child_range(SubExprs, SubExprs+END_EXPR+NumAssocs);
   }
-
+  const_child_range children() const {
+    return const_child_range(SubExprs, SubExprs + END_EXPR + NumAssocs);
+  }
   friend class ASTStmtReader;
 };
 

 
   // Iterators
   child_range children() { return child_range(&Base, &Base+1); }
+  const_child_range children() const {
+    return const_child_range(&Base, &Base + 1);
+  }
 };
 
 /// BlockExpr - Adaptor class for mixing a BlockDecl with expressions.

   child_range children() {
     return child_range(child_iterator(), child_iterator());
   }
+  const_child_range children() const {
+    return const_child_range(const_child_iterator(), const_child_iterator());
+  }
 };
 
 /// AsTypeExpr - Clang builtin function __builtin_astype [OpenCL 6.2.4.2]

 
   // Iterators
   child_range children() { return child_range(&SrcExpr, &SrcExpr+1); }
+  const_child_range children() const {
+    return const_child_range(&SrcExpr, &SrcExpr + 1);
+  }
 };
 
 /// PseudoObjectExpr - An expression which accesses a pseudo-object

   }
 
   child_range children() {
-    Stmt **cs = reinterpret_cast<Stmt**>(getSubExprsBuffer());
-    return child_range(cs, cs + getNumSubExprs());
+    const_child_range CCR =
+        const_cast<const PseudoObjectExpr *>(this)->children();
+    return child_range(cast_away_const(CCR.begin()),
+                       cast_away_const(CCR.end()));
   }
+  const_child_range children() const {
+    Stmt *const *cs = const_cast<Stmt *const *>(
+        reinterpret_cast<const Stmt *const *>(getSubExprsBuffer()));
+    return const_child_range(cs, cs + getNumSubExprs());
+  }
 
   static bool classof(const Stmt *T) {
     return T->getStmtClass() == PseudoObjectExprClass;

   child_range children() {
     return child_range(SubExprs, SubExprs+NumSubExprs);
   }
+  const_child_range children() const {
+    return const_child_range(SubExprs, SubExprs + NumSubExprs);
+  }
 };
 
 /// TypoExpr - Internal placeholder for expressions where typo correction

   child_range children() {
     return child_range(child_iterator(), child_iterator());
   }
+  const_child_range children() const {
+    return const_child_range(const_child_iterator(), const_child_iterator());
+  }
+
   SourceLocation getLocStart() const LLVM_READONLY { return SourceLocation(); }
   SourceLocation getLocEnd() const LLVM_READONLY { return SourceLocation(); }
   

include/clang/AST/StmtIterator.h

 
   StmtIterator(const VariableArrayType *t)
     : StmtIteratorImpl<StmtIterator,Stmt*&>(t) {}
+
+private:
+  StmtIterator(const StmtIteratorBase &RHS)
+      : StmtIteratorImpl<StmtIterator, Stmt *&>(RHS) {}
+
+  inline friend StmtIterator
+  cast_away_const(const struct ConstStmtIterator &RHS);
 };
 
 struct ConstStmtIterator : public StmtIteratorImpl<ConstStmtIterator,

 
   ConstStmtIterator(const StmtIterator& RHS) :
     StmtIteratorImpl<ConstStmtIterator,const Stmt*>(RHS) {}
+
+  ConstStmtIterator(Stmt * const *S)
+      : StmtIteratorImpl<ConstStmtIterator, const Stmt *>(
+            const_cast<Stmt **>(S)) {}
 };
 
+inline StmtIterator cast_away_const(const ConstStmtIterator &RHS) {
+  return RHS;
+}
 } // end namespace clang
 
 #endif

lib/AST/Expr.cpp

   }
   case InitListExprClass: {
     const InitListExpr *ILE = cast<InitListExpr>(this);
-    if (ILE->getType()->isArrayType()) {
+    QualType UnderlyingType = ILE->getType();
+    if (UnderlyingType->isAtomicType())
+      UnderlyingType = UnderlyingType->castAs<AtomicType>()->getValueType();
+
+    if (UnderlyingType->isArrayType()) {
       unsigned numInits = ILE->getNumInits();
       for (unsigned i = 0; i < numInits; i++) {
         if (!ILE->getInit(i)->isConstantInitializer(Ctx, false, Culprit))

       return true;
     }
 
-    if (ILE->getType()->isRecordType()) {
+    if (UnderlyingType->isRecordType()) {
       unsigned ElementNo = 0;
-      RecordDecl *RD = ILE->getType()->getAs<RecordType>()->getDecl();
+      RecordDecl *RD = UnderlyingType->getAs<RecordType>()->getDecl();
       for (const auto *Field : RD->fields()) {
         // If this is a union, skip all the fields that aren't being initialized.
         if (RD->isUnion() && ILE->getInitializedFieldInUnion() != Field)

 
 // UnaryExprOrTypeTraitExpr
 Stmt::child_range UnaryExprOrTypeTraitExpr::children() {
+  const_child_range CCR =
+      const_cast<const UnaryExprOrTypeTraitExpr *>(this)->children();
+  return child_range(cast_away_const(CCR.begin()), cast_away_const(CCR.end()));
+}
+
+Stmt::const_child_range UnaryExprOrTypeTraitExpr::children() const {
   // If this is of a type and the type is a VLA type (and not a typedef), the
   // size expression of the VLA needs to be treated as an executable expression.
   // Why isn't this weirdness documented better in StmtIterator?
   if (isArgumentType()) {
-    if (const VariableArrayType* T = dyn_cast<VariableArrayType>(
-                                   getArgumentType().getTypePtr()))
-      return child_range(child_iterator(T), child_iterator());
-    return child_range(child_iterator(), child_iterator());
+    if (const VariableArrayType *T =
+            dyn_cast<VariableArrayType>(getArgumentType().getTypePtr()))
+      return const_child_range(const_child_iterator(T), const_child_iterator());
+    return const_child_range(const_child_iterator(), const_child_iterator());
   }
-  return child_range(&Argument.Ex, &Argument.Ex + 1);
+  return const_child_range(&Argument.Ex, &Argument.Ex + 1);
 }
 
 AtomicExpr::AtomicExpr(SourceLocation BLoc, ArrayRef<Expr*> args,