Option parsing: implement decrement operators for arg_iterator

[grimar]

Summary

Implementing of decrement operators allows doing things like:

for (auto *Arg : llvm::reverse(Args.filtered(OPT_z)))

[dblaikie]

Any chance of using the llvm utility iterator_facade to simplify this class & provide the new functionality?

[davide]

ehm, what's your use case for iterating lld arguments in reverse order?

[grimar]

In D30570#692668, @davide wrote:

ehm, what's your use case for iterating lld arguments in reverse order?

Case is https://reviews.llvm.org/D30530#inline-264974.

-z linker option can have any text parameter: -z foo, -z boo, -z moo. In my case I am trying to support
-z text/-z notext. Processing only the last text/notext would be enough to know the result value for option.
I am using Args.filtered(OPT_z) to take all -z options now and was asked during review to iterate from backwards.

[davide]

I'm not entirely sure it's worth the complexity. This just allows the lld driver to return early.

[davide]

In other words, if you find other cases where this help, fair enough, but I'm reluctant to introduce new code in ArgList with a single consumer for cosmetic reasons only.

[grimar]

In D30570#692844, @davide wrote:

In other words, if you find other cases where this help, fair enough, but I'm reluctant to introduce new code in ArgList with a single consumer for cosmetic reasons only.

I see.

include/llvm/Option/ArgList.h

class arg_iterator {
   /// bother to factor this into two separate iterator implementations.
   //
   // FIXME: Make efficient; the idea is to provide efficient iteration over
   // all arguments which match a particular id and then just provide an
   // iterator combinator which takes multiple iterators which can be
   // efficiently compared and returns them in order.
   OptSpecifier Id0, Id1, Id2;
 
-  void SkipToNextArg();
+  void skipToArg(bool Forward);
 
 public:
   typedef Arg * const *                 value_type;
   typedef Arg * const &                 reference;
   typedef Arg * const *                 pointer;
   typedef std::forward_iterator_tag   iterator_category;
   typedef std::ptrdiff_t              difference_type;
 
   arg_iterator(SmallVectorImpl<Arg *>::const_iterator it, const ArgList &Args,
                OptSpecifier Id0 = 0U, OptSpecifier Id1 = 0U,
                OptSpecifier Id2 = 0U)
       : Current(it), Args(Args), Id0(Id0), Id1(Id1), Id2(Id2) {
-    SkipToNextArg();
+    skipToArg(true /*Forward*/);
   }
 
   operator const Arg*() { return *Current; }
   reference operator*() const { return *Current; }
   pointer operator->() const { return Current; }
 
   arg_iterator &operator++() {
     ++Current;
-    SkipToNextArg();
+    skipToArg(true /*Forward*/);
     return *this;
   }
 
   arg_iterator operator++(int) {
-    arg_iterator tmp(*this);
+    arg_iterator Ret(*this);
     ++(*this);
-    return tmp;
+    return Ret;
+  }
+
+  arg_iterator &operator--() {
+    --Current;
+    skipToArg(false /*Forward*/);
+    return *this;
+  }
+
+  arg_iterator operator--(int) {
+    arg_iterator Ret(*this);
+    --(*this);
+    return Ret;
   }
 
   friend bool operator==(arg_iterator LHS, arg_iterator RHS) {
     return LHS.Current == RHS.Current;
   }
   friend bool operator!=(arg_iterator LHS, arg_iterator RHS) {
     return !(LHS == RHS);
   }
public:
 
   const_reverse_iterator rbegin() const { return Args.rbegin(); }
   const_reverse_iterator rend() const { return Args.rend(); }
 
   arg_iterator filtered_begin(OptSpecifier Id0 = 0U, OptSpecifier Id1 = 0U,
                               OptSpecifier Id2 = 0U) const {
     return arg_iterator(Args.begin(), *this, Id0, Id1, Id2);
   }
-  arg_iterator filtered_end() const {
-    return arg_iterator(Args.end(), *this);
+  arg_iterator filtered_end(OptSpecifier Id0 = 0U, OptSpecifier Id1 = 0U,
+                            OptSpecifier Id2 = 0U) const {
+    return arg_iterator(Args.end(), *this, Id0, Id1, Id2);
   }
 
   iterator_range<arg_iterator> filtered(OptSpecifier Id0 = 0U,
                                         OptSpecifier Id1 = 0U,
                                         OptSpecifier Id2 = 0U) const {
-    return make_range(filtered_begin(Id0, Id1, Id2), filtered_end());
+    return make_range(filtered_begin(Id0, Id1, Id2),
+                      filtered_end(Id0, Id1, Id2));
   }
 
   /// @}
   /// @name Arg Removal
   /// @{
 
   /// eraseArg - Remove any option matching \p Id.
   void eraseArg(OptSpecifier Id);

lib/Option/ArgList.cpp

 #include "llvm/Option/Arg.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace llvm;
 using namespace llvm::opt;
 
-void arg_iterator::SkipToNextArg() {
-  for (; Current != Args.end(); ++Current) {
+void arg_iterator::skipToArg(bool Forward) {
+  auto End = Forward ? Args.end() : Args.begin();
+
+  while (Current != End) {
     // Done if there are no filters.
     if (!Id0.isValid())
       break;
 
     // Otherwise require a match.
     const Option &O = (*Current)->getOption();
-    if (O.matches(Id0) ||
-        (Id1.isValid() && O.matches(Id1)) ||
+    if (O.matches(Id0) || (Id1.isValid() && O.matches(Id1)) ||
         (Id2.isValid() && O.matches(Id2)))
       break;
+
+    Current += Forward ? 1 : -1;
   }
 }
 
 void ArgList::append(Arg *A) {
   Args.push_back(A);
 }
 
 void ArgList::eraseArg(OptSpecifier Id) {

unittests/Option/OptionParsingTest.cpp

TEST(Option, OptionParsing) {
   arg_iterator Cs = AL.filtered_begin(OPT_C);
   ASSERT_NE(AL.filtered_end(), Cs);
   EXPECT_EQ("desu", StringRef((*Cs)->getValue()));
   ArgStringList ASL;
   (*Cs)->render(AL, ASL);
   ASSERT_EQ(2u, ASL.size());
   EXPECT_EQ("-C", StringRef(ASL[0]));
   EXPECT_EQ("desu", StringRef(ASL[1]));
+
+  // Test iterator operators.
+  arg_iterator CsIt = AL.filtered_begin(OPT_C);
+  arg_iterator CsOp1 = CsIt++;
+  EXPECT_EQ("desu", StringRef((*CsOp1)->getValue()));
+  EXPECT_EQ("bye", StringRef((*CsIt)->getValue()));
+  arg_iterator CsOp2 = CsIt--;
+  EXPECT_EQ("bye", StringRef((*CsOp2)->getValue()));
+  EXPECT_EQ("desu", StringRef((*CsIt)->getValue()));
+  arg_iterator CsOp3 = ++CsIt;
+  EXPECT_EQ("bye", StringRef((*CsOp3)->getValue()));
+  EXPECT_EQ("bye", StringRef((*CsIt)->getValue()));
+  arg_iterator CsOp4 = --CsIt;
+  EXPECT_EQ("desu", StringRef((*CsOp4)->getValue()));
+  EXPECT_EQ("desu", StringRef((*CsIt)->getValue()));
+  arg_iterator CsItEnd = AL.filtered_end(OPT_C);
+  EXPECT_EQ("bye", StringRef((*--CsItEnd)->getValue()));
 }
 
 TEST(Option, ParseWithFlagExclusions) {
   TestOptTable T;
   unsigned MAI, MAC;
 
   // Exclude flag3 to avoid parsing as OPT_SLASH_C.
   InputArgList AL = T.ParseArgs(Args, MAI, MAC,