[llvm] r226939 - Move the accessor functions from DIExpression::iterator into a wrapper
David Blaikie
dblaikie at gmail.com
Fri Jan 23 14:32:26 PST 2015
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.
> 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?)
> 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.
> iterator &operator++() {
> increment();
> return *this;
> @@ -889,14 +890,7 @@ public:
> bool operator==(const iterator &X) const { return I == X.I; }
> bool operator!=(const iterator &X) const { return !(*this == X); }
>
> - uint64_t getArg(unsigned N) const {
> - auto In = I;
> - std::advance(In, N);
> - return In.getNumber<uint64_t>();
> - }
> -
> const DIHeaderFieldIterator &getBase() const { return I; }
> -
> private:
> void increment() {
> switch (**this) {
> @@ -911,6 +905,22 @@ public:
>
> iterator begin() const;
> iterator end() const;
> +
> + /// \brief A lightweight wrapper around an element of a DIExpression.
> + class Operand {
> + DIHeaderFieldIterator I;
> + public:
> + Operand(DIHeaderFieldIterator I) : I(I) {}
>
should this be private & friendedly accessible from the expr iterator?
> + /// \brief Operands such as DW_OP_piece have explicit (non-stack)
> arguments.
> + /// Argument 0 is the operand itself.
> + uint64_t getArg(unsigned N) const {
> + DIHeaderFieldIterator In = I;
> + std::advance(In, N);
> + return In.getNumber<uint64_t>();
> + }
> +
> + operator uint64_t () const { return I.getNumber<uint64_t>(); }
>
Maybe this should be an accessor with a name (& type safety?) rather than
an implicit conversion?
> + };
> };
>
> /// \brief This object holds location information.
>
> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfExpression.cpp?rev=226939&r1=226938&r2=226939&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfExpression.cpp (original)
> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfExpression.cpp Fri Jan 23
> 15:24:41 2015
> @@ -210,8 +210,8 @@ bool DwarfExpression::AddMachineRegExpre
> switch (*I) {
>
Can the loop this is within, be turend into a range-based-for loop so
everyone doesn't have to do dereference the iterator.
If not, at least all the "(*I)." below could change to "I->" instead?
> case dwarf::DW_OP_piece: {
> unsigned SizeOfByte = 8;
> - unsigned OffsetInBits = I.getArg(1) * SizeOfByte;
> - unsigned SizeInBits = I.getArg(2) * SizeOfByte;
> + unsigned OffsetInBits = (*I).getArg(1) * SizeOfByte;
> + unsigned SizeInBits = (*I).getArg(2) * SizeOfByte;
> // Piece always comes at the end of the expression.
> return AddMachineRegPiece(MachineReg, SizeInBits,
> getOffsetOrZero(OffsetInBits, PieceOffsetInBits));
> @@ -219,7 +219,7 @@ bool DwarfExpression::AddMachineRegExpre
> case dwarf::DW_OP_plus:
> // [DW_OP_reg,Offset,DW_OP_plus,DW_OP_deref] --> [DW_OP_breg,Offset].
> if (*std::next(I) == dwarf::DW_OP_deref) {
> - unsigned Offset = I.getArg(1);
> + unsigned Offset = (*I).getArg(1);
> ValidReg = AddMachineRegIndirect(MachineReg, Offset);
> std::advance(I, 2);
> break;
> @@ -248,14 +248,14 @@ void DwarfExpression::AddExpression(DIEx
> switch (*I) {
> case dwarf::DW_OP_piece: {
> unsigned SizeOfByte = 8;
> - unsigned OffsetInBits = I.getArg(1) * SizeOfByte;
> - unsigned SizeInBits = I.getArg(2) * SizeOfByte;
> + unsigned OffsetInBits = (*I).getArg(1) * SizeOfByte;
> + unsigned SizeInBits = (*I).getArg(2) * SizeOfByte;
> AddOpPiece(SizeInBits, getOffsetOrZero(OffsetInBits,
> PieceOffsetInBits));
> break;
> }
> case dwarf::DW_OP_plus:
> EmitOp(dwarf::DW_OP_plus_uconst);
> - EmitUnsigned(I.getArg(1));
> + EmitUnsigned((*I).getArg(1));
> break;
> case dwarf::DW_OP_deref:
> EmitOp(dwarf::DW_OP_deref);
>
> Modified: llvm/trunk/lib/IR/DebugInfo.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DebugInfo.cpp?rev=226939&r1=226938&r2=226939&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/IR/DebugInfo.cpp (original)
> +++ llvm/trunk/lib/IR/DebugInfo.cpp Fri Jan 23 15:24:41 2015
> @@ -1406,16 +1406,15 @@ void DIVariable::printInternal(raw_ostre
> }
>
> void DIExpression::printInternal(raw_ostream &OS) const {
> - for (auto E = end(), I = begin(); I != E; ++I) {
> - uint64_t OpCode = *I;
> - OS << " [" << OperationEncodingString(OpCode);
> - switch (OpCode) {
> + for (auto Op : *this) {
> + OS << " [" << OperationEncodingString(Op);
> + switch (Op) {
> case DW_OP_plus: {
> - OS << " " << I.getArg(1);
> + OS << " " << Op.getArg(1);
> break;
> }
> case DW_OP_piece: {
> - OS << " offset=" << I.getArg(1) << ", size=" << I.getArg(2);
> + OS << " offset=" << Op.getArg(1) << ", size=" << Op.getArg(2);
> break;
> }
> case DW_OP_deref:
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150123/26ead4ba/attachment.html>
More information about the llvm-commits
mailing list