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

For many and fold, consider ..0 an invalid range, just as 0..0 is #1632

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cky
Copy link
Contributor

@cky cky commented Jan 31, 2023

nom::multi::many and nom::many::fold consider empty ranges, as checked by NomRange::is_inverted, to be invalid. Unfortunately, that method is incorrectly implemented for RangeTo, meaning that ..0 is not considered invalid when it should be.

While I'm here, I also fixed up off-by-one errors in RangeFrom::bounded_iter and RangeFull::bounded_iter.

@cky cky requested a review from Geal as a code owner January 31, 2023 02:45
@coveralls
Copy link

Pull Request Test Coverage Report for Build 4050233552

  • 6 of 9 (66.67%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.3%) to 62.55%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/traits.rs 6 9 66.67%
Files with Coverage Reduction New Missed Lines %
src/character/streaming.rs 1 78.38%
Totals Coverage Status
Change from base Build 3981619336: 0.3%
Covered Lines: 1560
Relevant Lines: 2494

💛 - Coveralls

Copy link
Contributor

@cenodis cenodis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching these. It has been a long while since I last touched this piece of the code but the off-by-one fixes seem correct to me. I'm just not sure about removing the explicit self.end == 0 checks. Those still seem necessary to me.

if self.end == 0 {
1..0
} else {
0..self.end - 1
}
0..self.end - 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this panic when overflow checking is enabled (such as when building with debug)?
And even if the underflow does not panic it would return 0..usize::MAX as the iterator for a range of x..0. This is incorrect because the range should be empty. Similar reasoning applies to the other range implementations.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 4050233552

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 6 of 9 (66.67%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.3%) to 62.55%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/traits.rs 6 9 66.67%
Files with Coverage Reduction New Missed Lines %
src/character/streaming.rs 1 78.38%
Totals Coverage Status
Change from base Build 3981619336: 0.3%
Covered Lines: 1560
Relevant Lines: 2494

💛 - Coveralls

1 similar comment
@coveralls
Copy link

Pull Request Test Coverage Report for Build 4050233552

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 6 of 9 (66.67%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.3%) to 62.55%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/traits.rs 6 9 66.67%
Files with Coverage Reduction New Missed Lines %
src/character/streaming.rs 1 78.38%
Totals Coverage Status
Change from base Build 3981619336: 0.3%
Covered Lines: 1560
Relevant Lines: 2494

💛 - Coveralls

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.

3 participants