<algorithm>: Use iter[idx] for clarity

[lozinska]

Description

Replace *(iter+idx) to iter[idx] for clarity #278

Checklist

Be sure you've read README.md and understand the scope of this repo.

If you're unsure about a box, leave it unchecked. A maintainer will help you.

• Identifiers in product code changes are properly _Ugly as per
  https://eel.is/c++draft/lex.name#3.1 or there are no product code changes.
• The STL builds successfully and all tests have passed (must be manually
  verified by an STL maintainer before automated testing is enabled on GitHub,
  leave this unchecked for initial submission).
• These changes introduce no known ABI breaks (adding members, renaming
  members, adding virtual functions, changing whether a type is an aggregate
  or trivially copyable, etc.).
• These changes were written from scratch using only this repository and
  the C++ Working Draft as a reference (and any other cited standards).
  If they were derived from a project that's already listed in NOTICE.txt,
  that's fine, but please mention it. If they were derived from any other
  project (including Boost and libc++, which are not yet listed in
  NOTICE.txt), you must mention it here, so we can determine whether the
  license is compatible and what else needs to be done.

[StephanTLavavej]

Whitespace changes are needed to pass clang-format validation; other than that, these changes look good to me.

[CaseyCarter]

I'm happy to merge this without test changes; defending against regression of clarity seems like a hard problem.

[StephanTLavavej]

I'm porting this to our Microsoft-internal MSVC repo now, for a full build and test run.

[BillyONeal]

As I said, I'm still concerned about backcompat fallout; as long as RWC comes back OK I agree this is nicer.

[StephanTLavavej]

Thanks for implementing this enhancement! MSVC's internal test suites have passed, so I've merged your change for the VS 2019 16.5 Preview 2 release. Another test suite of "Real World Code" projects will run (it takes longer), compiling various open-source programs with the updated STL. If we encounter breaking changes there, we'll submit fixes to those projects and document this as a significant breaking change (although I think that's unlikely).

[StephanTLavavej]

Fix merged: facebook/rocksdb#6047

[tobias-loew]

This fix has some interesting side-effects with boost::iterator_facade.
For example, sorting a boost::multi_array<double,2>with std::sort doesn't work anymore in VS 2019 16.5 Preview 2.
The reason is that operator[] returns the internal class boost::iterators::detail::operator_brackets_proxy which cannot be converted to the iterators value_type as that would require two user-defined conversions.
This could be fixed by adding an additional "operator value_type() const" conversion-operator to the class operator_brackets_proxy.
But even then, std::sort will now call its predicate-function with instances of boost::iterators::detail::operator_brackets_proxy, (before it called with the reference_type of boost::multi_array<...> which is boost::detail::multi_array::sub_array<...>)

[CaseyCarter]

Formally Cpp17RandomAccessIterator requires that a[n] be implicitly convertible to the iterator's reference type, whereas [algorithms.requirements] requires that Predicate and BinaryPredicate work when "applied to the result of dereferencing the corresponding iterator" (respectively "applied to the result of dereferencing two corresponding iterators or to dereferencing an iterator and type T when T is part of the signature [presumably this means to say const T, e.g., std::lower_bound]". Together, the wording implies that we cannot use e.g. _Pred(_UFirst[-1], _Val) in search_n since _UFirst[-1] hasn't been implicitly converted to the iterator's reference type.

I am forcefully reminded that half of the reason for using Concepts in Ranges was to clean up the horrid mess that is the legacy algorithm requirements.

[CaseyCarter]

(Aside: I don't know how much we should concern ourselves specifically with operator_brackets_proxy, which exists to support "iterators that may invalidate referents upon destruction" which don't meet the forward iterator requirements.)

[BillyONeal]

IMO we should just ban operator[] if there are any algorithms for which there are problems. (i.e. revert this PR)

[StephanTLavavej]

Those Predicate requirements are an unfortunate surprise to me. Although I thought that subscripting improved clarity, I agree that we should revert this PR and enhance our test coverage. I'll take care of that.

[tobias-loew]

thanks a lot to all

[IRebo]

Sorry if this is not the right place to ask but 16.5.0 was released still breaking boost::iterator_facade. Will there be a 16.5.x update that fixes this ?

[BillyONeal]

Probably not, #464 was merged well after the 16.5 lockdown.