[llvm] r226747 - Rewrite DIExpression::Verify() using an iterator. NFC.

[Duncan P. N. Exon Smith]

> On 2015-Jan-21, at 16:00, Adrian Prantl <aprantl at apple.com> wrote:
> 
> Author: adrian
> Date: Wed Jan 21 18:00:52 2015
> New Revision: 226747
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=226747&view=rev
> Log:
> Rewrite DIExpression::Verify() using an iterator. NFC.
> 
> Addresses review comments for r226627.
> 

This is a nice cleanup, thanks.  It inspired me to clean up the header
iterator itself (see r22677{0,2,3,4,5}).

Also, one comment below...

> Modified:
>    llvm/trunk/include/llvm/IR/DebugInfo.h
>    llvm/trunk/lib/IR/DebugInfo.cpp
> 
> Modified: llvm/trunk/include/llvm/IR/DebugInfo.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/DebugInfo.h?rev=226747&r1=226746&r2=226747&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/IR/DebugInfo.h (original)
> +++ llvm/trunk/include/llvm/IR/DebugInfo.h Wed Jan 21 18:00:52 2015
> @@ -59,7 +59,7 @@ class DIObjCProperty;
> typedef DenseMap<const MDString *, MDNode *> DITypeIdentifierMap;
> 
> class DIHeaderFieldIterator
> -    : public std::iterator<std::input_iterator_tag, StringRef, std::ptrdiff_t,
> +    : public std::iterator<std::forward_iterator_tag, StringRef, std::ptrdiff_t,
>                            const StringRef *, StringRef> {
>   StringRef Header;
>   StringRef Current;
> @@ -834,6 +834,8 @@ public:
>   void printExtendedName(raw_ostream &OS) const;
> };
> 
> +class DIExpressionIterator;
> +
> /// \brief A complex location expression.
> class DIExpression : public DIDescriptor {
>   friend class DIDescriptor;
> @@ -862,8 +864,64 @@ public:
>   uint64_t getPieceOffset() const;
>   /// \brief Return the size of this piece in bytes.
>   uint64_t getPieceSize() const;
> +
> +  DIExpressionIterator begin() const;
> +  DIExpressionIterator end() const;
> };
> 
> +/// \brief An iterator for DIExpression elments.
> +class DIExpressionIterator

I feel like this should just be defined inside `DIExpression` with the
name `iterator`.  Thoughts?

> +    : public std::iterator<std::forward_iterator_tag, StringRef, unsigned,
> +                           const uint64_t *, uint64_t> {
> +  DIHeaderFieldIterator I;
> +  DIExpressionIterator(DIHeaderFieldIterator I) : I(I) {}
> +public:
> +  DIExpressionIterator() {}
> +  DIExpressionIterator(const DIExpression Expr)
> +    : I(Expr.getHeader()) { ++I; }
> +  uint64_t operator*() const {
> +    uint64_t UInt;
> +    if (I->getAsInteger(0, UInt))
> +      return 0;
> +    return UInt;
> +  }
> +  DIExpressionIterator &operator++() {
> +    increment();
> +    return *this;
> +  }
> +  DIExpressionIterator operator++(int) {
> +    DIExpressionIterator X(*this);
> +    increment();
> +    return X;
> +  }
> +  bool operator==(const DIExpressionIterator &X) const {
> +    return I == X.I;
> +  }
> +  bool operator!=(const DIExpressionIterator &X) const {
> +    return !(*this == X);
> +  }
> +
> +  uint64_t getArg(unsigned N) const {
> +    auto In = I;
> +    std::advance(In, N);
> +    return *DIExpressionIterator(In);
> +  }
> +
> +  const DIHeaderFieldIterator& getBase() const { return I; }
> +
> +private:
> +  void increment() {
> +    switch (**this) {
> +    case dwarf::DW_OP_piece: std::advance(I, 3); break;
> +    case dwarf::DW_OP_plus:  std::advance(I, 2); break;
> +    case dwarf::DW_OP_deref: std::advance(I, 1); break;
> +    default:
> +      assert("unsupported operand");
> +    }
> +  }
> +};
> +
> +
> /// \brief This object holds location information.
> ///
> /// This object is not associated with any DWARF tag.
> 
> Modified: llvm/trunk/lib/IR/DebugInfo.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DebugInfo.cpp?rev=226747&r1=226746&r2=226747&view=diff
> ==============================================================================
> --- llvm/trunk/lib/IR/DebugInfo.cpp (original)
> +++ llvm/trunk/lib/IR/DebugInfo.cpp Wed Jan 21 18:00:52 2015
> @@ -162,6 +162,14 @@ uint64_t DIExpression::getPieceSize() co
>   return getElement(getNumElements()-1);
> }
> 
> +DIExpressionIterator DIExpression::begin() const {
> +  return DIExpressionIterator(*this);
> +}
> +
> +DIExpressionIterator DIExpression::end() const {
> +  return DIExpressionIterator();
> +}
> +
> //===----------------------------------------------------------------------===//
> // Predicates
> //===----------------------------------------------------------------------===//
> @@ -595,25 +603,25 @@ bool DIExpression::Verify() const {
>   if (!DbgNode)
>     return true;
> 
> -  unsigned N = getNumElements();
> -  for (unsigned I = 0; I < N; ++I)
> -    switch (getElement(I)) {
> +  if (!(isExpression() && DbgNode->getNumOperands() == 1))
> +    return false;
> +
> +  for (auto E = end(), I = begin(); I != E; ++I)
> +    switch (*I) {
>     case DW_OP_piece:
> -      // DW_OP_piece has to be the last element in the expression and take two
> -      // arguments.
> -      if (getElement(I) == DW_OP_piece && !isVariablePiece())
> -        return false;
> -      I += 2;
> -      break;
> +      // Must be the last element of the expression.
> +      return std::distance(I.getBase(), DIHeaderFieldIterator()) == 3;
>     case DW_OP_plus:
> -      // Takes one argument.
> -      if (I+1 == N)
> +      if (std::distance(I.getBase(), DIHeaderFieldIterator()) < 2)
>         return false;
> -      I += 1;
>       break;
> -    default: break;
> -  }
> -  return isExpression() && DbgNode->getNumOperands() == 1;
> +    case DW_OP_deref:
> +      break;
> +    default:
> +      // Other operators are not yet supported by the backend.
> +      return false;
> +    }
> +  return true;
> }
> 
> bool DILocation::Verify() const {
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits