[llvm] r226939 - Move the accessor functions from DIExpression::iterator into a wrapper

[David Blaikie]

On Fri, Jan 23, 2015 at 3:01 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

>
> > On 2015-Jan-23, at 14:32, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> >
> > On Fri, Jan 23, 2015 at 1:24 PM, Adrian Prantl <aprantl at apple.com>
> wrote:
> > Author: adrian
> > Date: Fri Jan 23 15:24:41 2015
> > New Revision: 226939
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=226939&view=rev
> > Log:
> > Move the accessor functions  from DIExpression::iterator into a wrapper
> > DIExpression::Operand, so we can write range-based for loops.
> >
> > Thanks to David Blaikie for the idea.
> >
> > Modified:
> >     llvm/trunk/include/llvm/IR/DebugInfo.h
> >     llvm/trunk/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
> >     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=226939&r1=226938&r2=226939&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/include/llvm/IR/DebugInfo.h (original)
> > +++ llvm/trunk/include/llvm/IR/DebugInfo.h Fri Jan 23 15:24:41 2015
> > @@ -867,16 +867,17 @@ public:
> >    /// \brief Return the size of this piece in bytes.
> >    uint64_t getPieceSize() const;
> >
> > +  class Operand;
> > +
> >    /// \brief An iterator for DIExpression elements.
> >    class iterator : public std::iterator<std::forward_iterator_tag,
> StringRef,
> >                                          unsigned, const uint64_t *,
> uint64_t> {
> >
> > I'm guessing these types passed to std::iterator need to be updated now
> that the value is an Operand, not a uint64_t.
> >
>
> Also, this should be a input_iterator, shouldn't it?
>
> >      DIHeaderFieldIterator I;
> >
> > Really vaguely curious - should the expression be encoded as a raw byte
> sequence, rather than bytes in strings in a DIHeader? (bytes in strings
> seems inefficient?)
> >
>
> I think this is fine as is as a stop-gap.  When my work is finished
> (I'm stalling because I'm having trouble figuring out how to stage
> everything), DIExpression won't be backed by a string: it'll store an
> array of `uint64_t` directly.
>
> >      iterator(DIHeaderFieldIterator I) : I(I) {}
> > -
> >    public:
> >      iterator() {}
> >      iterator(const DIExpression &Expr) : I(++Expr.header_begin()) {}
> > -    uint64_t operator*() const { return I.getNumber<uint64_t>(); }
> > +    Operand operator*() const { return Operand(I); }
> >
> > I /think/ you might need to return by reference than by value here (&
> have an Operand value as a member of the iterator) - I think that the
> iterator requirements require a reference to be returned or some other
> feature that makes returning by value not viable - but I could totally be
> wrong here, my memory is very vague.
>
> Ah, right.  This is what I was thinking of above.
>
> If we switch to input_iterator, returning by value is fine.
>

I seemed to recall it wasn't OK even for input iterators.

Looking at the spec, it seems input iterator still has to support -> and to
do that you need storage (since -> returns a raw pointer to the data).


>
> (Anyone know what happened to the "new iterator concepts" [1] that
> fixed this nonsense of conflating traversal with value access?
>
> [1]:
> http://www.boost.org/doc/libs/1_57_0/libs/iterator/doc/new-iter-concepts.html
> )
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150123/2936f73f/attachment.html>