[PATCH] D31171: Improve StringMap iterator support.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Seems weird to have 'keys' on a StringSet, perhaps? (& to test StringSet in the StringMap unit test - as much as I realize the two types are very closely related)

Also, I'm wondering if keys() is necessary even on StringMap - I'd have thought a general-purpose "iterator of the first element of a pair" would suffice, but then remembered that StringMaps are special and their values are not pair<key, value> but have special accessors for the key, etc. So perhaps a lambda-driven iterator adapter... at which point, yeah, I can understand the utility of having that wrapped up in a reusable form.



================
Comment at: llvm/include/llvm/ADT/StringExtras.h:255
+template <typename Range, typename Sep>
+inline std::string join_range(Range &&R, Sep Separator) {
+  return join(R.begin(), R.end(), Separator);
----------------
most of the other "iterator pair or range" algorithms use the same name for both (eg: all the llvm::any/all/none_of clones that take a range use the same name) - maybe use the same name here too ('join' rather than 'join_range') unless that causes some ambiguities?

Also - this seems like an unrelated change for this patch (& arguably could use test coverage if it is going to be committed). Guess maybe it slipped into this change.


================
Comment at: llvm/include/llvm/ADT/StringMap.h:474
 
-  bool operator==(const StringMapConstIterator &RHS) const {
-    return Ptr == RHS.Ptr;
-  }
-  bool operator!=(const StringMapConstIterator &RHS) const {
-    return Ptr != RHS.Ptr;
-  }
+  bool operator==(const DerivedTy &RHS) const { return Ptr == RHS.Ptr; }
 
----------------
Generally it's advised that any op overload that can be a non-member should be, to allow equal implicit conversions on both sides. (it can still be a friend, though:

  friend bool operator==(LHS, RHS) { return LHS.Ptr == RHS.Ptr; }


================
Comment at: llvm/include/llvm/ADT/StringMap.h:533
+
+  StringRef operator*() const { return I->getKey(); }
+};
----------------
op* that returns by value - does that cause problems for op->? I forget... 


================
Comment at: llvm/unittests/Support/StringMap.cpp:38
+
+  auto Keys = to_vector<4>(Set.keys());
+  std::sort(Keys.begin(), Keys.end());
----------------
In retrospect, maybe this function should be called To(Small)Vector? (Since there's no standard library function it's trying to emulate, etc - and it actually produces a SmallVector, not a std::vector, so the name is a bit vague/misleading (& makes the int non-type template parameter unclear/confusing))


https://reviews.llvm.org/D31171