[PATCH] D113158: ADT: Fix const-correctness of iterator adaptors

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

In D113158#3122372 <https://reviews.llvm.org/D113158#3122372>, @dexonsmith wrote:

> In D113158#3120023 <https://reviews.llvm.org/D113158#3120023>, @dblaikie wrote:
>
>>> There are lots of custom iterators that *only* define a non-const qualified `operator*()`. In some cases, it'd make sense to make a member `mutable`, or change the return type to always be `const`-qualified. But perhaps not in every case?
>>
>> I think that covers all cases I can think of, and seems like the right thing - but if it means lots of cleanup, and just keeping/making two overloads (const/non-const) that both return non-const ref (well, return ref, whatever that is - const ref for const_iterators (that themselves may or may not be const... you know what I mean)) is significantly cheaper right now - I'd be OK with that. Seems like it'd make things incrementally better at least - leave a comment aobut it being a stop-gap, not a complete solution, maybe some comments about the functionality or patterns that are deprecated/urge people towards just writing the one const op*?
>
> I think I'd rather commit as-is with just one `operator*() const` if you agree it's the right answer. (I found the counter-examples by doing a grep for `operator*() {`, but they aren't being used in iterator adaptors right now... so I don't think we need a stop-gap.)

Yep, I'm on board!

> Think there's value in adding a test that a second operator non-const-qualified operator isn't added as a place to add a comment, something like:
>
>   // There should only be one `operator*()` and it should be const-qualified.
>   // Rather than adding overloads, fields in adapted iterators should be made
>   // mutable or ReferenceT should be `const`-qualified.
>   auto x = &decltype(make_filter_range(...).begin())::operator*;
>   auto x = &decltype(make_map_range(...).begin())::operator*;
>   auto x = &decltype(reverse(...).begin())::operator*;
>   ...
>
> ?

Can we put that in the iterator adapter base somehow, so we don't have to make it a statement about each use of iterator adapters?


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

https://reviews.llvm.org/D113158