<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Tue, Mar 21, 2017 at 8:13 AM Zachary Turner via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">zturner added inline comments.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: llvm/include/llvm/ADT/StringExtras.h:255<br class="gmail_msg">
+template <typename Range, typename Sep><br class="gmail_msg">
+inline std::string join_range(Range &&R, Sep Separator) {<br class="gmail_msg">
+  return join(R.begin(), R.end(), Separator);<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> 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?<br class="gmail_msg">
><br class="gmail_msg">
> 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.<br class="gmail_msg">
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`.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: llvm/include/llvm/ADT/StringMap.h:533<br class="gmail_msg">
+<br class="gmail_msg">
+  StringRef operator*() const { return I->getKey(); }<br class="gmail_msg">
+};<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> op* that returns by value - does that cause problems for op->? I forget...<br class="gmail_msg">
Should be fine, the `StringRef` should live until the end of the sequence point.<br class="gmail_msg"></blockquote><div><br>Depends which sequence point. If you take a look at iterator_facade_base:<br><br><div>  PointerT operator->() const {</div><div>    return &static_cast<const DerivedT *>(this)->operator*();</div><div>  }<br><br>This would end up returning a pointer to a local StringRef - the StringRef would cease to exist/be valid at the end of the return evaluation, leading to UB in the caller.</div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
<br class="gmail_msg">
<a href="https://reviews.llvm.org/D31171" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D31171</a><br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
</blockquote></div></div>