[PATCH] D102760: [llvm] Let SmallVector construct from any Iterable

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.


================
Comment at: llvm/include/llvm/ADT/IterableTraits.h:17-31
+namespace detail {
+
+template <typename I> auto is_forward_iterator(...) -> std::false_type;
+
+template <typename I>
+auto is_forward_iterator(int) -> decltype(
+    std::next(std::declval<I>()),
----------------
Instead of all this, I would recommend
```
template<class It>
using is_input_iterator = std::is_base_of<std::input_iterator_tag, typename std::iterator_traits<It>::iterator_category>;

template<class R>
using iterator_type_of = std::decay_t<decltype(std::begin(std::declval<R>()))>;

template<class R, class IsInputIterable = std::true_type>
struct is_range_iterable : std::false_type {};

template<class R>
struct is_range_iterable<R, typename is_input_iterator<iterator_type_of<R>>::type> : std::true_type {};
```
(Notice that we're supposed to be checking for //input// iterability, not //forward// iterability. But also, the right way to see if something is an input iterator is not to check an ad-hoc bunch of operators like `*` and `!=`, but simply to check its STL iterator category.)

However, in real life, I personally would reject this patch, because I think "saving a couple of calls to begin/end at the call site" is not worth the peril of letting people accidentally write e.g.
```
SmallVector<SomeKindaString> v("hello world");
assert(v.size() == 12);  // elements are "h"s, "e"s, "l"s, etc.
```


================
Comment at: llvm/include/llvm/ADT/SmallVector.h:1191-1194
+  template <typename Iterable>
+  explicit SmallVector(
+      Iterable &&Range,
+      std::enable_if_t<llvm::is_range_iterable<Iterable>::value, bool> = true)
----------------
Watch out: this greedy constructor template will be used for non-const copy construction, e.g.

    SmallVector<int> x;
    SmallVector<int> y = x;  // calls SmallVector<int>::SmallVector<SmallVector<int>&>(SmallVector<int>&, bool),
    // which is a better match than SmallVector<int>::SmallVector(const SmallVector<int>&)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102760/new/

https://reviews.llvm.org/D102760