[PATCH] D64462: [ADT] [WIP] Add MutableRange class

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I think you're trying to do too much in this patch. Having an idea of how it could be extended is very different from going ahead and implementing it, and I really don't see a need to add support for all the types you are currently supporting. Certainly, I think this is too big to review as is.

I know I suggested bringing it across here, but @MaskRay is probably right in that we probably need another concrete use case for this code to put it into ADT. I think it would be wise to scale it back to what is needed for the MutableObject patch series, sorry.

Rather than completely throwing this all away, maybe you should create a new patch that just keeps it tightly focused to what is needed (perhaps similar to what you had before), referencing this patch, and then abandoning this patch. Then, if someone in the future feels there's a need for a more general solution, this patch can be resurrected.



================
Comment at: llvm/include/llvm/ADT/MutableRange.h:29
+template <typename T, typename = void>
+class is_associative : public std::false_type {};
+template <typename T>
----------------
Things like this probably belong in a separate TypeTraits header or similar.


================
Comment at: llvm/include/llvm/ADT/MutableRange.h:32
+class is_associative<
+    T, typename std::enable_if<sizeof(typename T::key_type) != 0>::type>
+    : public std::true_type {};
----------------
It might be worth adding an implementation of C++ 17's `void_t` to make this, is_map and is_unordered much cleaner:

```template <typename T>
class is_associative<T, void_t<T::key_type>> : public std::true_type {};```


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

https://reviews.llvm.org/D64462