[PATCH] D65788: [ADT] Remove O(1) requirement to range passed to llvm::size

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D65788#1616674 <https://reviews.llvm.org/D65788#1616674>, @abrachet wrote:

> In D65788#1616378 <https://reviews.llvm.org/D65788#1616378>, @jhenderson wrote:
>
> > How many places use this and in what context? I'm conscious of std::size which becomes available in C++17, and the natural implementation of that is to simply use the standard .size() method on a container (see https://en.cppreference.com/w/cpp/iterator/size). It also does something else for fixed-sized arrays, but this particular version doesn't work for that. Finally, why do you need llvm::size() instead of a container.size() method?
>
>
> I was also curious about the use of `llvm::size()` instead of the containers size method, seems like ranges which can be computed in O(1) are almost always going to be in containers which have a size method. My particular use is with the iterators in `ObjectFile::sections(), symbols()` etc. which are not in a container with a size method and may not even be contiguous in memory.
>
> I've added @dblaikie as a subscriber as it seems from D46976 <https://reviews.llvm.org/D46976> it was his idea to give this restriction to `llvm::size()`.


I'm still inclined to stick with my original sentiment, that this shouldn't be usable on non-constant-time size operations.

Changing llvm::size to match C++20 is going to move the problem - then we'd need to define a "size()" member of llvm::iterator_range & have the same debate there about whether it should be limited to O(1) cases or not. (it /sounds/ like the C++20 ranges will have such a limit but I'm not entirely sure/familiar with them)

My preference would be to use std::distance(begin, end) for O(N) size computations to make it clear that they're "interesting", as I mentioned in the original review/discussion.


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

https://reviews.llvm.org/D65788