[PATCH] D31171: Improve StringMap iterator support.

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
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);
----------------
dblaikie wrote:
> 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.
Yea I almost called it `join`, but then saw that we have `join` and `join_items` so `join_range` seemed to fit nicely.  That said, `join_items` was introduced because of an ambiguity with `join`, but `join_range` won't have that ambiguity, so we could still call it `join`.


================
Comment at: llvm/include/llvm/ADT/StringMap.h:533
+
+  StringRef operator*() const { return I->getKey(); }
+};
----------------
dblaikie wrote:
> op* that returns by value - does that cause problems for op->? I forget... 
Should be fine, the `StringRef` should live until the end of the sequence point.


https://reviews.llvm.org/D31171