r242608 - [AST] Cleanup ExprIterator.
Benjamin Kramer
benny.kra at gmail.com
Mon Jul 27 10:26:17 PDT 2015
> On 27.07.2015, at 19:09, Alexandros Tzannes <atzannes at illinois.edu> wrote:
>
> Hi Benjamin,
> is there an advantage to making ExprIterator and ConstExprIterator protected in Stmt?
>
> I have some methods that takes arguments of type ExpIterator and used to work for several different classes derived from Stmt (e.g., CXXConstructExpr, CallExpr, ...). Now that Stmt::ExprIterator is protected the code does not compile anymore. (E.g., void foo(ExprIterator ArgI, ExprIterator ArgE) which basically factors out a loop over expressions).
>
> Right now, I think my best bet is to make these methods generic (template for type EXPR_ITERATOR) and have it specialized for the different public typedefs of ExprIterator (CXXConstructExpr::arg_iterator, CallExpr::arg_iterator). Is there a better approach? I'm also thinking of making ExprIterator public in the private branch of Clang that I'm working on.
Why not just use CallExpr::arg_iterator and rely on it being the same as CXXConstructExpr::arg_iterator? There is no reason for ExprIterator being protected, other than it being an implementation detail of the Stmt class hierarchy, using the typedefs is preferable. If you want something truly generic there is always child_begin()/child_end().
- Ben
>
> On 07/18/2015 05:35 PM, Benjamin Kramer wrote:
>> Author: d0k
>> Date: Sat Jul 18 09:35:53 2015
>> New Revision: 242608
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=242608&view=rev
>> Log:
>> [AST] Cleanup ExprIterator.
>>
>> - Make it a proper random access iterator with a little help from iterator_adaptor_base
>> - Clean up users of magic dereferencing. The iterator should behave like an Expr **.
>> - Make it an implementation detail of Stmt. This allows inlining of the assertions.
>>
>> Modified:
>> cfe/trunk/include/clang/AST/Stmt.h
>> cfe/trunk/lib/AST/Expr.cpp
>> cfe/trunk/lib/AST/StmtPrinter.cpp
>> cfe/trunk/lib/CodeGen/CGCall.cpp
>> cfe/trunk/lib/CodeGen/CGClass.cpp
>> cfe/trunk/lib/CodeGen/CGExprCXX.cpp
>> cfe/trunk/lib/CodeGen/CodeGenFunction.h
>> cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp
>>
>> Modified: cfe/trunk/include/clang/AST/Stmt.h
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Stmt.h?rev=242608&r1=242607&r2=242608&view=diff
>> ==============================================================================
>> --- cfe/trunk/include/clang/AST/Stmt.h (original)
>> +++ cfe/trunk/include/clang/AST/Stmt.h Sat Jul 18 09:35:53 2015
>> @@ -22,6 +22,7 @@
>> #include "clang/Basic/SourceLocation.h"
>> #include "llvm/ADT/ArrayRef.h"
>> #include "llvm/ADT/PointerIntPair.h"
>> +#include "llvm/ADT/iterator.h"
>> #include "llvm/Support/Compiler.h"
>> #include "llvm/Support/ErrorHandling.h"
>> #include <string>
>> @@ -49,57 +50,6 @@ namespace clang {
>> class Token;
>> class VarDecl;
>> - //===--------------------------------------------------------------------===//
>> - // ExprIterator - Iterators for iterating over Stmt* arrays that contain
>> - // only Expr*. This is needed because AST nodes use Stmt* arrays to store
>> - // references to children (to be compatible with StmtIterator).
>> - //===--------------------------------------------------------------------===//
>> -
>> - class Stmt;
>> - class Expr;
>> -
>> - class ExprIterator : public std::iterator<std::forward_iterator_tag,
>> - Expr *&, ptrdiff_t,
>> - Expr *&, Expr *&> {
>> - Stmt** I;
>> - public:
>> - ExprIterator(Stmt** i) : I(i) {}
>> - ExprIterator() : I(nullptr) {}
>> - ExprIterator& operator++() { ++I; return *this; }
>> - ExprIterator operator-(size_t i) { return I-i; }
>> - ExprIterator operator+(size_t i) { return I+i; }
>> - Expr* operator[](size_t idx);
>> - // FIXME: Verify that this will correctly return a signed distance.
>> - signed operator-(const ExprIterator& R) const { return I - R.I; }
>> - Expr* operator*() const;
>> - Expr* operator->() const;
>> - bool operator==(const ExprIterator& R) const { return I == R.I; }
>> - bool operator!=(const ExprIterator& R) const { return I != R.I; }
>> - bool operator>(const ExprIterator& R) const { return I > R.I; }
>> - bool operator>=(const ExprIterator& R) const { return I >= R.I; }
>> - };
>> -
>> - class ConstExprIterator : public std::iterator<std::forward_iterator_tag,
>> - const Expr *&, ptrdiff_t,
>> - const Expr *&,
>> - const Expr *&> {
>> - const Stmt * const *I;
>> - public:
>> - ConstExprIterator(const Stmt * const *i) : I(i) {}
>> - ConstExprIterator() : I(nullptr) {}
>> - ConstExprIterator& operator++() { ++I; return *this; }
>> - ConstExprIterator operator+(size_t i) const { return I+i; }
>> - ConstExprIterator operator-(size_t i) const { return I-i; }
>> - const Expr * operator[](size_t idx) const;
>> - signed operator-(const ConstExprIterator& R) const { return I - R.I; }
>> - const Expr * operator*() const;
>> - const Expr * operator->() const;
>> - bool operator==(const ConstExprIterator& R) const { return I == R.I; }
>> - bool operator!=(const ConstExprIterator& R) const { return I != R.I; }
>> - bool operator>(const ConstExprIterator& R) const { return I > R.I; }
>> - bool operator>=(const ConstExprIterator& R) const { return I >= R.I; }
>> - };
>> -
>> //===----------------------------------------------------------------------===//
>> // AST classes for statements.
>> //===----------------------------------------------------------------------===//
>> @@ -337,6 +287,39 @@ public:
>> /// de-serialization).
>> struct EmptyShell { };
>> +protected:
>> + /// Iterator for iterating over Stmt * arrays that contain only Expr *
>> + ///
>> + /// This is needed because AST nodes use Stmt* arrays to store
>> + /// references to children (to be compatible with StmtIterator).
>> + struct ExprIterator
>> + : llvm::iterator_adaptor_base<ExprIterator, Stmt **,
>> + std::random_access_iterator_tag, Expr *> {
>> + ExprIterator() : iterator_adaptor_base(nullptr) {}
>> + ExprIterator(Stmt **I) : iterator_adaptor_base(I) {}
>> +
>> + reference operator*() const {
>> + assert((*I)->getStmtClass() >= firstExprConstant &&
>> + (*I)->getStmtClass() <= lastExprConstant);
>> + return *reinterpret_cast<Expr **>(I);
>> + }
>> + };
>> +
>> + /// Const iterator for iterating over Stmt * arrays that contain only Expr *
>> + struct ConstExprIterator
>> + : llvm::iterator_adaptor_base<ConstExprIterator, const Stmt *const *,
>> + std::random_access_iterator_tag,
>> + const Expr *const> {
>> + ConstExprIterator() : iterator_adaptor_base(nullptr) {}
>> + ConstExprIterator(const Stmt *const *I) : iterator_adaptor_base(I) {}
>> +
>> + reference operator*() const {
>> + assert((*I)->getStmtClass() >= firstExprConstant &&
>> + (*I)->getStmtClass() <= lastExprConstant);
>> + return *reinterpret_cast<const Expr *const *>(I);
>> + }
>> + };
>> +
>> private:
>> /// \brief Whether statistic collection is enabled.
>> static bool StatisticsEnabled;
>>
>> Modified: cfe/trunk/lib/AST/Expr.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Expr.cpp?rev=242608&r1=242607&r2=242608&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/AST/Expr.cpp (original)
>> +++ cfe/trunk/lib/AST/Expr.cpp Sat Jul 18 09:35:53 2015
>> @@ -4163,19 +4163,6 @@ PseudoObjectExpr::PseudoObjectExpr(QualT
>> }
>> //===----------------------------------------------------------------------===//
>> -// ExprIterator.
>> -//===----------------------------------------------------------------------===//
>> -
>> -Expr* ExprIterator::operator[](size_t idx) { return cast<Expr>(I[idx]); }
>> -Expr* ExprIterator::operator*() const { return cast<Expr>(*I); }
>> -Expr* ExprIterator::operator->() const { return cast<Expr>(*I); }
>> -const Expr* ConstExprIterator::operator[](size_t idx) const {
>> - return cast<Expr>(I[idx]);
>> -}
>> -const Expr* ConstExprIterator::operator*() const { return cast<Expr>(*I); }
>> -const Expr* ConstExprIterator::operator->() const { return cast<Expr>(*I); }
>> -
>> -//===----------------------------------------------------------------------===//
>> // Child Iterators for iterating over subexpressions/substatements
>> //===----------------------------------------------------------------------===//
>>
>> Modified: cfe/trunk/lib/AST/StmtPrinter.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/StmtPrinter.cpp?rev=242608&r1=242607&r2=242608&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/AST/StmtPrinter.cpp (original)
>> +++ cfe/trunk/lib/AST/StmtPrinter.cpp Sat Jul 18 09:35:53 2015
>> @@ -1768,7 +1768,7 @@ void StmtPrinter::VisitCXXTemporaryObjec
>> for (CXXTemporaryObjectExpr::arg_iterator Arg = Node->arg_begin(),
>> ArgEnd = Node->arg_end();
>> Arg != ArgEnd; ++Arg) {
>> - if (Arg->isDefaultArgument())
>> + if ((*Arg)->isDefaultArgument())
>> break;
>> if (Arg != Node->arg_begin())
>> OS << ", ";
>>
>> Modified: cfe/trunk/lib/CodeGen/CGCall.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCall.cpp?rev=242608&r1=242607&r2=242608&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/CodeGen/CGCall.cpp (original)
>> +++ cfe/trunk/lib/CodeGen/CGCall.cpp Sat Jul 18 09:35:53 2015
>> @@ -2829,7 +2829,7 @@ void CodeGenFunction::EmitCallArgs(CallA
>> for (int I = ArgTypes.size() - 1; I >= 0; --I) {
>> CallExpr::const_arg_iterator Arg = ArgBeg + I;
>> EmitCallArg(Args, *Arg, ArgTypes[I]);
>> - EmitNonNullArgCheck(Args.back().RV, ArgTypes[I], Arg->getExprLoc(),
>> + EmitNonNullArgCheck(Args.back().RV, ArgTypes[I], (*Arg)->getExprLoc(),
>> CalleeDecl, ParamsToSkip + I);
>> }
>> @@ -2843,7 +2843,7 @@ void CodeGenFunction::EmitCallArgs(CallA
>> CallExpr::const_arg_iterator Arg = ArgBeg + I;
>> assert(Arg != ArgEnd);
>> EmitCallArg(Args, *Arg, ArgTypes[I]);
>> - EmitNonNullArgCheck(Args.back().RV, ArgTypes[I], Arg->getExprLoc(),
>> + EmitNonNullArgCheck(Args.back().RV, ArgTypes[I], (*Arg)->getExprLoc(),
>> CalleeDecl, ParamsToSkip + I);
>> }
>> }
>>
>> Modified: cfe/trunk/lib/CodeGen/CGClass.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGClass.cpp?rev=242608&r1=242607&r2=242608&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/CodeGen/CGClass.cpp (original)
>> +++ cfe/trunk/lib/CodeGen/CGClass.cpp Sat Jul 18 09:35:53 2015
>> @@ -1832,7 +1832,7 @@ CodeGenFunction::EmitSynthesizedCXXCopyC
>> "trivial 1-arg ctor not a copy/move ctor");
>> EmitAggregateCopyCtor(This, Src,
>> getContext().getTypeDeclType(D->getParent()),
>> - E->arg_begin()->getType());
>> + (*E->arg_begin())->getType());
>> return;
>> }
>> llvm::Value *Callee = CGM.getAddrOfCXXStructor(D, StructorType::Complete);
>>
>> Modified: cfe/trunk/lib/CodeGen/CGExprCXX.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprCXX.cpp?rev=242608&r1=242607&r2=242608&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/CodeGen/CGExprCXX.cpp (original)
>> +++ cfe/trunk/lib/CodeGen/CGExprCXX.cpp Sat Jul 18 09:35:53 2015
>> @@ -196,7 +196,7 @@ RValue CodeGenFunction::EmitCXXMemberOrO
>> // Trivial move and copy ctor are the same.
>> assert(CE->getNumArgs() == 1 && "unexpected argcount for trivial ctor");
>> llvm::Value *RHS = EmitLValue(*CE->arg_begin()).getAddress();
>> - EmitAggregateCopy(This, RHS, CE->arg_begin()->getType());
>> + EmitAggregateCopy(This, RHS, (*CE->arg_begin())->getType());
>> return RValue::get(This);
>> }
>> llvm_unreachable("unknown trivial member function");
>> @@ -1089,8 +1089,7 @@ RValue CodeGenFunction::EmitBuiltinNewDe
>> bool IsDelete) {
>> CallArgList Args;
>> const Stmt *ArgS = Arg;
>> - EmitCallArgs(Args, *Type->param_type_begin(),
>> - ConstExprIterator(&ArgS), ConstExprIterator(&ArgS + 1));
>> + EmitCallArgs(Args, *Type->param_type_begin(), &ArgS, &ArgS + 1);
>> // Find the allocation or deallocation function that we're calling.
>> ASTContext &Ctx = getContext();
>> DeclarationName Name = Ctx.DeclarationNames
>>
>> Modified: cfe/trunk/lib/CodeGen/CodeGenFunction.h
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenFunction.h?rev=242608&r1=242607&r2=242608&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/CodeGen/CodeGenFunction.h (original)
>> +++ cfe/trunk/lib/CodeGen/CodeGenFunction.h Sat Jul 18 09:35:53 2015
>> @@ -2992,7 +2992,7 @@ public:
>> .getCanonicalType((*I).getNonReferenceType())
>> .getTypePtr() ==
>> getContext()
>> - .getCanonicalType(Arg->getType())
>> + .getCanonicalType((*Arg)->getType())
>> .getTypePtr())) &&
>> "type mismatch in call argument!");
>> ArgTypes.push_back(*I);
>>
>> Modified: cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp?rev=242608&r1=242607&r2=242608&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp (original)
>> +++ cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp Sat Jul 18 09:35:53 2015
>> @@ -3803,9 +3803,8 @@ MicrosoftCXXABI::getAddrOfCXXCtorClosure
>> CodeGenFunction::RunCleanupsScope Cleanups(CGF);
>> const auto *FPT = CD->getType()->castAs<FunctionProtoType>();
>> - ConstExprIterator ArgBegin(ArgVec.data()),
>> - ArgEnd(ArgVec.data() + ArgVec.size());
>> - CGF.EmitCallArgs(Args, FPT, ArgBegin, ArgEnd, CD, IsCopy ? 1 : 0);
>> + CGF.EmitCallArgs(Args, FPT, &*ArgVec.begin(), &*ArgVec.end(), CD,
>> + IsCopy ? 1 : 0);
>> // Insert any ABI-specific implicit constructor arguments.
>> unsigned ExtraArgs = addImplicitConstructorArgs(CGF, CD, Ctor_Complete,
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
More information about the cfe-commits
mailing list