[PATCH] D31171: Improve StringMap iterator support.

[David Blaikie via llvm-commits]

On Tue, Mar 21, 2017 at 8:13 AM Zachary Turner via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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.
>

Depends which sequence point. If you take a look at iterator_facade_base:

  PointerT operator->() const {
    return &static_cast<const DerivedT *>(this)->operator*();
  }

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.


>
>
> https://reviews.llvm.org/D31171
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170321/3560d7d9/attachment.html>