Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

<execution>: comment early return control flow #818 #849

Merged
merged 6 commits into from
May 30, 2020

Conversation

AlexGuteniev
Copy link
Contributor

Resolves #818

@AlexGuteniev AlexGuteniev requested a review from a team as a code owner May 19, 2020 18:08
@StephanTLavavej StephanTLavavej added the documentation Related to documentation or comments label May 19, 2020
stl/inc/execution Outdated Show resolved Hide resolved
@AlexGuteniev
Copy link
Contributor Author

A question about which places need put this comment.

So far I found 11 places with early return:

STL/stl/inc/execution

Lines 1289 to 1299 in ff94756

_Cancellation_status _Process_chunk() {
if (_Results._Complete()) {
return _Cancellation_status::_Canceled;
}
const auto _Key = _Team._Get_next_key();
if (!_Key) {
return _Cancellation_status::_Canceled;
}
const auto _Range = _Basis._Get_chunk(_Key);

There are also places without early return:

STL/stl/inc/execution

Lines 1418 to 1425 in ff94756

_Cancellation_status _Process_chunk() {
const auto _Key = _Team._Get_next_key();
if (!_Key) {
return _Cancellation_status::_Canceled;
}
const auto _Chunk_number = _Key._Chunk_number;
const auto _Range = _Basis._Get_chunk(_Key);

Should these be commented as well?

@StephanTLavavej
Copy link
Member

StephanTLavavej commented May 27, 2020

Should these be commented as well?

I think so. I'm not an expert here, but the difference appears to be that some algorithms can return early (e.g. all_of can return early upon finding false) but other algorithms must perform all work (e.g. transform must transform all elements). Either way, once an algorithm obtains work, I don't believe it can abandon that work.

Edited to add: Hmm, after looking at all occurrences of _Get_next_key, I think perhaps we should comment just the places with early returns (which is what you're doing now), as those are the patterns I was originally interested in refactoring. I think commenting all callsites could be distracting or misleading.

@StephanTLavavej StephanTLavavej self-assigned this May 29, 2020
@StephanTLavavej StephanTLavavej merged commit c4c4821 into microsoft:master May 30, 2020
@StephanTLavavej
Copy link
Member

Thanks for making it easier to understand this code, and to write correct code in the future! 🎉

@AlexGuteniev AlexGuteniev deleted the execf branch May 30, 2020 03:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Related to documentation or comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<execution>: Consider commenting the _Results._Complete() control flow
3 participants