Fix a selection of const related issues with ranget.
#3833
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thomasspriggs
requested review from
kroening,
peterschrammel,
pkesseli,
smowton and
tautschnig
as code owners
|
I would squash this slightly more, e.g., put the test into the commit of the change. |
|
Also, I'd change the return types of for consistency. |
kroening
approved these changes
Jan 18, 2019
allredj
reviewed
Jan 18, 2019
There was a problem hiding this comment.
✔️
Passed Diffblue compatibility checks (cbmc commit: 7a5ff90).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/97863971
allredj
reviewed
Jan 18, 2019
There was a problem hiding this comment.
✔️
Passed Diffblue compatibility checks (cbmc commit: 20cc2dd).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/97872239
smowton
approved these changes
Jan 19, 2019
unit/util/range.cpp
Outdated
| [&](const std::string &s) { return s.length() == 4; }); | ||
| auto it = filtered_range.begin(); | ||
| REQUIRE(*it == "cdef"); | ||
| ++it; | ||
| REQUIRE(it == filtered_range.end()); | ||
| } | ||
| THEN("A const filter iterator can mutate the input collection.") |
There was a problem hiding this comment.
Suggest rephrasing, since "const filter iterator" sounds an awful lot like a const_iterator, which this is not
Dereferencing a `filter_iteratort` from filtering a const collection with `ranget` would previously introduce a compile error, due to a `const` to non-const conversion. This commit fixes this, by using the same return type as the iterator which it wraps around. This commit includes a test for using range to filter a `const` collection.
Dereferencing a `concat_iteratort` from concating const collections with `ranget` would previously introduce a compile error, due to a `const` to non-const conversion. This commit fixes this, by using the same return type as the iterator which it wraps around. This commit includes a test for using range to concat `const` collections.
It was previously impossible to iterate over a `const` range, because you couldn't call `.begin()` on it. This fixes that issue. Changing this to `const` does not allow the internals of the range to be mutated, as it returns a copy of the begin iterator, not a reference to it. This commit adds `const` to ranges in range unit test to show that ranges can be made const correct, using this fix.
Algorithms which are written expecting a STL container may reference a `value_type` member, as this is the name of the member in STL containers. Therefore this needs to be named the same way in `ranget` in order to be compatible with such algorithms. This commit includes a unit test of `ranget` having correct the `value_type` defined.
Given that `filter` and `concat` didn't work with const input collections, it seems prudent to test that `map` works with const input collections as well. There is no corresponding fix to make this test pass in this PR.
Deferencing a `const` iterator which refers to a non-const value should enable mutation of that value. This is consistent with how iterators in the STL work. It means that we only need to provide a single const overload of these operators, not both the const and non-const variations. So the benefit of this commit is better consistency with the STL and fewer operator overloads to maintain.
thomasspriggs
force-pushed
the
tas/range_constness
branch
from
20cc2dd to
a89ca18
Compare
allredj
reviewed
Jan 19, 2019
There was a problem hiding this comment.
✔️
Passed Diffblue compatibility checks (cbmc commit: a89ca18).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/97929087
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Attempting to use
rangeton const input collections or attempting to use aconst rangetwould previously cause compile errors. This PR fixes these issues.