[PATCH] D129988: [WIP] Drop SmallVector constructor taking iterator_range in favor of llvm::to_vector/llvm::to_vector_of

[Dawid Jurczak via Phabricator via llvm-commits]

yurai007 added a comment.

In D129988#3659583 <https://reviews.llvm.org/D129988#3659583>, @dexonsmith wrote:

> 



> Since the change has such a broad impact, this might deserve a discussion on a discourse topic. It’d be less work and introduce way less churn to add an ArrayRef overload to SmallVector’s list of constructors, and maybe that would accomplish enough?

I agree that adding ArrayRef overload would be much easier and less intrusive to code base. Maybe we should start with that.

> If this change does go ahead (I have a weak opinion that it should but would be fine with adding ArrayRef instead), then it should be broken up somehow into more incremental patches, converting small groups of uses in chunks small enough to be reviewed/reverted, and with the removal of the constructor in a final patch on its own (which out of tree users could easily temporarily revert to give them time to convert their out of tree code).

Well, I don't have strong opinion either. However the more I think about idea of dropping iterator_range constructor the less I like it. 
Both SmallVector and Iterator_range are heavily used in LLVM sources and removing constructor has big impact on whole tree (as this change proves).
Although to_vector/to_vector_of are great utilities and using them in many cases simplify (or at least reduce verbosity) still there are cases when using  plain constructor seem to be better idea, like for mentioned type alias case:

VectRet Res = VectRet(detail::reverse_if<!InverseEdge>(R));

In general it would be great to give user full freedom to decide which to_vector* or constructor approach is right way for particular usage. Using constructor still looks for me like most idiomatic way for object creation in C++.
One of motivation for dropping SmallVector(const iterator_range<RangeTy>&) was referring to std::vector as one which doesn't have similar range constructor.
However in contrast to LLVM iterator_range, C++ Ranges is pretty new C++20 feature that starts gaining popularity.
Interestingly there is proposal for adding range constructor to many stdlib containers: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p1206r7.pdf already approved for inclusion to C++23.
Also I checked last git history and I couldn't find any similar "iterator_range removal" patches which makes me wonder... that after all maybe what we need is just adding ArrayRef constructor :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129988