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

New method range_proxy::size() to know the size of the range. #3

Merged
merged 6 commits into from
Nov 21, 2020

Conversation

Alzathar
Copy link
Contributor

No description provided.

@klmr
Copy link
Owner

klmr commented May 28, 2018

Hi,

thanks, this is a good idea. However, the proposed change only adds the size member to the default range_proxy without a step size. It should also be added to the range_proxy::step_range_proxy to keep the interfaces consistent.

@Alzathar
Copy link
Contributor Author

I can add the member size()for the class range_proxy::step_range_proxywith the following formula:

return static_cast<size_t>(std::floor((*end_ - *begin_) / step_));

@klmr
Copy link
Owner

klmr commented May 28, 2018

Unfortunately this yields wrong results (the correct result in all cases is 4):

  • range(1, 8).step(2).size() = 3
  • range(8.0, 1.0).step(-2.0).size() = 18446744073709551615
  • range(8, 1).step(-2).size() = 0

@Alzathar
Copy link
Contributor Author

Alzathar commented May 28, 2018

From my understanding the result should be 3 because the range is [1,8[. The number 8 cannot be reached because it is associated with the end() iterator. (I should have think harder before writing this sentence...)

The code below is an update for the method range_proxy::step_range_proxy and take into account all possible cases (increasing/decreasing/empty range with positive/negative/null step).

// null step
if (step_ == T(0)) {
    return static_cast<size_t>(-1);
}
// increasing and null range
if (*end_ >= *begin_) {
    if (step_ < T(0)) {
        return 0;
    }
    return static_cast<size_t>(std::ceil(double(*end_ - *begin_) / double(step_));
}
// decreasing range
if (*end_ < *begin_) {
    if (step_ > T(0)) {
        return 0;
    }
    return static_cast<size_t>(std::ceil(double(*begin_ - *end_) / double(step_));
}

I did not test this code in C++, but from tests in another language it will give your the result 4 in your examples. The code range(0.1, 0.11).step(2).size() returns 1.

Finally, the formula used in the range_proxy::size() method should be updated to take into account the case where the range is not valid (i.e. *end_ < *begin):

if (*end_ < *begin_) {
    return 0;
}
return static_cast<size_t>(*end_ - *begin_);

@klmr
Copy link
Owner

klmr commented May 28, 2018

From my understanding the result should be 3 because the range is [1,8[. The number 8 cannot be reached because it is associated with the end() iterator.

Right, 8 cannot be reached; so the output is 1, 3, 5, 7, which is four different numbers. And analogously for the other ranges.

@Alzathar
Copy link
Contributor Author

You are right... I updated the code in my previous comment to compute the good size.

@klmr
Copy link
Owner

klmr commented May 28, 2018

Looks good. Except the step_ == 0 case, that’s not meaningful and should probably be caught at instantiation.

Anyway, if you write a commit I’ll merge it!

Implementation updated from the PR discussion.
Tests were added to verify the results
@Alzathar
Copy link
Contributor Author

@klmr I finally implemented the range_proxy::step_range_proxy::size() now available in this PR. I updated the code as we discussed and I tested it in the file test.cpp.

* Simplify `size` implementation
    Author: Konrad Rudolph <konrad.rudolph@gmail.com>

* Use name “iterator” consistently
    Author: Konrad Rudolph <konrad.rudolph@gmail.com>

* Method range_proxy::step_range_proxy::size()
    * Implementation updated from the PR discussion.
    * Tests were added to verify the results
    Author: Arnaud Barré <alzathar@Moveck-C05.local>

* New method range_proxy::size() to know the size of the range.
    Author: Arnaud Barré <arnaud.barre@gmail.com>
@klmr
Copy link
Owner

klmr commented Nov 21, 2020

Wow, what a coincidence: almost exactly a year after your last change. Apologies for the delay!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants