<algorithm>: Use iter[idx] for clarity by lozinska · Pull Request #289 · microsoft/STL

lozinska · 2019-11-12T06:01:50Z

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.

stl/inc/algorithm

CaseyCarter

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

StephanTLavavej · 2019-11-15T23:05:46Z

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 · 2019-11-16T01:12:29Z

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 · 2019-11-21T06:08:59Z

Fix merged: facebook/rocksdb#6047

tobias-loew · 2020-01-27T16:05:56Z

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 · 2020-01-27T19:42:55Z

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 · 2020-01-27T19:46:55Z

(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 · 2020-01-27T21:39:24Z

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

StephanTLavavej · 2020-01-28T00:59:24Z

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.

This reverts microsoft#289 and changes several more algorithms. Unrelated cleanup: this changes one occurrence of `[[nodiscard]]` to `_NODISCARD` for consistency.

tobias-loew · 2020-01-28T07:01:46Z

thanks a lot to all

This reverts #289 and changes several more algorithms. Unrelated cleanup: this changes one occurrence of `[[nodiscard]]` to `_NODISCARD` for consistency.

IRebo · 2020-03-20T15:50:48Z

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 · 2020-03-20T17:54:01Z

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

lozinska added 4 commits October 25, 2019 02:39

Fix issue-218 - remove public redundancy

874f646

Fix issue-218 - removing 'public' redundancy

1a76aa7

Merge branch 'master' of https://github.com/microsoft/STL into issue-218

607e6f4

Fix microsoft#278 - change to iter[idx] for clarity

dc676c3

lozinska requested a review from a team as a code owner November 12, 2019 06:01

StephanTLavavej changed the title ~~Issue 278~~ <algorithm>: Use iter[idx] for clarity Nov 13, 2019

StephanTLavavej requested changes Nov 13, 2019

View reviewed changes

stl/inc/algorithm Outdated Show resolved Hide resolved

stl/inc/algorithm Outdated Show resolved Hide resolved

stl/inc/algorithm Outdated Show resolved Hide resolved

stl/inc/algorithm Outdated Show resolved Hide resolved

stl/inc/algorithm Outdated Show resolved Hide resolved

lozinska added 3 commits November 13, 2019 23:53

Fix microsoft#278 add space alligning

84ee3d0

Fix microsoft#278 add space alligning

f773ea5

Fix microsoft#278 add space alligning

1dbf015

lozinska requested a review from StephanTLavavej November 14, 2019 16:03

StephanTLavavej approved these changes Nov 14, 2019

View reviewed changes

StephanTLavavej requested review from BillyONeal, CaseyCarter and barcharcraz November 14, 2019 19:28

CaseyCarter approved these changes Nov 14, 2019

View reviewed changes

StephanTLavavej merged commit 1648f3d into microsoft:master Nov 16, 2019

BillyONeal approved these changes Nov 16, 2019

View reviewed changes

tobias-loew mentioned this pull request Jan 27, 2020

added conversion to value_type to operator_brackets_proxy boostorg/iterator#54

Closed

StephanTLavavej mentioned this pull request Jan 28, 2020

<algorithm>: Algorithms shouldn't use operator[] with iterators #463

Closed

StephanTLavavej mentioned this pull request Jan 28, 2020

Fix #463 by avoiding iterator subscripting. #464

Merged

4 tasks

StephanTLavavej added a commit that referenced this pull request Jan 28, 2020

Fix #463 by avoiding iterator subscripting. (#464)

2bd2bd2

This reverts #289 and changes several more algorithms. Unrelated cleanup: this changes one occurrence of `[[nodiscard]]` to `_NODISCARD` for consistency.

Conversation

lozinska commented Nov 12, 2019 • edited by BillyONeal Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

StephanTLavavej left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CaseyCarter left a comment

Choose a reason for hiding this comment

Uh oh!

StephanTLavavej commented Nov 15, 2019

Uh oh!

BillyONeal left a comment

Choose a reason for hiding this comment

Uh oh!

StephanTLavavej commented Nov 16, 2019

Uh oh!

StephanTLavavej commented Nov 21, 2019

Uh oh!

tobias-loew commented Jan 27, 2020

Uh oh!

CaseyCarter commented Jan 27, 2020

Uh oh!

CaseyCarter commented Jan 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BillyONeal commented Jan 27, 2020

Uh oh!

StephanTLavavej commented Jan 28, 2020

Uh oh!

tobias-loew commented Jan 28, 2020

Uh oh!

IRebo commented Mar 20, 2020

Uh oh!

BillyONeal commented Mar 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lozinska commented Nov 12, 2019 •

edited by BillyONeal

Loading

CaseyCarter commented Jan 27, 2020 •

edited

Loading