[libcxx-commits] [PATCH] D142302: [libc++][format] Fixes usage of contiguous ranges.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked an inline comment as done.
Mordante added a comment.

Thanks for the feedback!



================
Comment at: libcxx/include/__format/range_formatter.h:175
+              ranges::data(__range),
+              ranges::size(__range),
+          },
----------------
huixie90 wrote:
> Mordante wrote:
> > JMazurkiewicz wrote:
> > > How about using `ranges::distance` (http://eel.is/c++draft/range.iter.op.distance#4) here instead of `ranges::size`? We could get rid of `ranges::sized_range<_Rp>` constraint.
> > > 
> > > Also, I think we need extra includes: `<__ranges/data.h>` and `<__ranges/size.h>` (or `<__iterator/distance.h>`).
> > I expected `ranges::distance` to require a `sized_range`, but I didn't check it. But you're right distance would work too and have less requirements.
> > 
> > Indeed these headers should be included. I'm surprised it passes the modular tests without them. 
> > 
> > 
> I disagree. Although it is really rare to see a `contiguous_range` that is not `sized_range`. But when this happens, I think we do want to constrain it to `sized_range`, otherwise `ranges::distance` is going to be a linear operation.  so using `ranges::size` is the right thing
I don't think the linear part will be a big issue; it's still faster than the alternative of copying the range in a temporary buffer. Still I will change it; distance returns a signed value, which runs into narrowing.

(I consider having a `contiguous_range` that is not a `sized_range` too rare to add a special case. This entire optimization isn't Standard mandated.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142302