Add ranget drop and skip methods #4347

romainbrenguier · 2019-03-07T17:53:08Z

This adds two utility methods to ranget which I would find useful.
For instance we could iterate on a ranget with a while loop instead of a range-for: while(!range.empty()) { ....; range.drop(1); ...}

Each commit message has a non-empty body, explaining why the change was made.
Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
[na] The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
[na] My commit message includes data points confirming performance improvements (if claimed).
My PR is restricted to a single feature or bugfix.
[na] White-space or formatting changes outside the feature-related changed lines are in commits of their own.

allredj

⚠️
This PR failed Diffblue compatibility checks (cbmc commit: 73cbe05).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/103580996
Status will be re-evaluated on next push.
Common spurious failures:

the cbmc commit has disappeared in the mean time (e.g. in a force-push)
the author is not in the list of contributors (e.g. first-time contributors).
the compatibility was already broken by an earlier merge.

tautschnig · 2019-03-07T19:34:05Z

src/util/range.h

+  void drop(std::size_t count)
+  {
+    std::size_t i = 0;
+    while(i < count && begin_value != end_value)


Nit pick, but wouldn't this make for a very nice for loop? As is, it looks a bit like assembler programming.

tautschnig · 2019-03-07T19:36:03Z

src/util/range.h

+  /// Return an new range containing the same elements except for the first
+  /// \p count elements.
+  /// If the range has fewer elements, returns an empty range.
+  ranget<iteratort> skip(std::size_t count) const


I don't find the names drop and skip entirely intuitive. I am wondering whether using longer, more unambiguous names would be better?

Maybe. I'm open to suggestions. drop is also used by range-v3, so we should maybe keep this one.

Whatever is used elsewhere I'm happy with, just stick with that terminology then.

drop and skip are both fine names, but they are kinda synonyms. I think we should choose one or the other, but not both. The name used in .net's linq is skip - https://docs.microsoft.com/en-us/dotnet/api/system.linq.enumerable.skip?view=netframework-4.7.2
The name in haskell would be drop - http://zvon.org/other/haskell/Outputprelude/drop_f.html
Because C++ supports overloading we could actually have 3 different overloads, all with the same name if we wanted. A mutation overload, a const & overload and an r-value overload. I would suggest keeping just a const & and the r-value overload, in order to reduce the need to reason about mutation.

tautschnig · 2019-03-07T19:37:31Z

unit/util/range.cpp

+    }
+    THEN("Skip first 2 elements")
+    {
+      auto range = make_range(list).skip(2);


I think it would be better to have a test where you also test the original range to make that indeed skip does not modify anything, just generates a copy.

romainbrenguier · 2019-03-07T20:41:16Z

@smowton I made a slight change to the drop function: by returning a Boolean we can avoid some redundant iterator comparison and for instance write while(range.drop(1)) { ... }

allredj

⚠️
This PR failed Diffblue compatibility checks (cbmc commit: 39146d2).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/103603097
Status will be re-evaluated on next push.
Common spurious failures:

the cbmc commit has disappeared in the mean time (e.g. in a force-push)
the author is not in the list of contributors (e.g. first-time contributors).
the compatibility was already broken by an earlier merge.

tautschnig · 2019-03-08T08:00:34Z

src/util/range.h

+  /// \return true if the range still contains element after the drop
+  bool drop(std::size_t count)
+  {
+    for(std::size_t i = 0; i < count; ++i)


You don't actually need i, do you? for( ; count > 0; --count) ... would do the trick.

allredj

⚠️
This PR failed Diffblue compatibility checks (cbmc commit: ac09326).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/103676521
Status will be re-evaluated on next push.
Common spurious failures:

the cbmc commit has disappeared in the mean time (e.g. in a force-push)
the author is not in the list of contributors (e.g. first-time contributors).
the compatibility was already broken by an earlier merge.

thomasspriggs · 2019-03-08T14:04:33Z

src/util/range.h

+  /// \return true if the range still contains element after the drop
+  ranget<iteratort> drop(std::size_t count) &&
+  {
+    for(; count > 0 && begin_value != end_value; --count)


💡 Could this be defined in terms of std::advance and std::distance? I think you could advance the begin iterator by the std::min of count and the distance between the begin and the end iterators.

Maybe, but I don't know if it's worth it. Some distance functions work by iterating from the current position to the end, so using that plus advance would end up being less efficient.

allredj

⚠️
This PR failed Diffblue compatibility checks (cbmc commit: 2f9f9e2).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/103685752
Status will be re-evaluated on next push.
Common spurious failures:

the cbmc commit has disappeared in the mean time (e.g. in a force-push)
the author is not in the list of contributors (e.g. first-time contributors).
the compatibility was already broken by an earlier merge.

romainbrenguier · 2019-03-08T14:11:53Z

Following @thomasspriggs suggestions, I made the two versions be named drop and return a new range, the difference between the two is that one works on ranges that are rvalues

This is useful for advancing the range without having to manipulate the private `begin_value` field. We have two versions of this, both return a new range, but one is used when the range on which the method is called can be moved.

This tests that these operations behave as expected and gives examples on how to use them.

This can avoid some copies of the iterators.

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: 962c9f4).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/103695677

romainbrenguier requested review from kroening, peterschrammel, pkesseli, smowton and tautschnig as code owners March 7, 2019 17:53

smowton approved these changes Mar 7, 2019

View reviewed changes

allredj reviewed Mar 7, 2019

View reviewed changes

tautschnig reviewed Mar 7, 2019

View reviewed changes

tautschnig assigned romainbrenguier Mar 7, 2019

romainbrenguier force-pushed the feature/range-drop branch from 73cbe05 to 39146d2 Compare March 7, 2019 20:39

allredj reviewed Mar 7, 2019

View reviewed changes

tautschnig reviewed Mar 8, 2019

View reviewed changes

romainbrenguier force-pushed the feature/range-drop branch 2 times, most recently from ac09326 to d6b5acb Compare March 8, 2019 11:52

allredj reviewed Mar 8, 2019

View reviewed changes

romainbrenguier force-pushed the feature/range-drop branch 2 times, most recently from 2f9f9e2 to e6cb484 Compare March 8, 2019 13:58

thomasspriggs reviewed Mar 8, 2019

View reviewed changes

allredj reviewed Mar 8, 2019

View reviewed changes

romainbrenguier added 3 commits March 8, 2019 14:13

Add drop functions for ranget

b81bbcb

This is useful for advancing the range without having to manipulate the private `begin_value` field. We have two versions of this, both return a new range, but one is used when the range on which the method is called can be moved.

Unit test for ranget drop

e80340d

This tests that these operations behave as expected and gives examples on how to use them.

Use move in ranget constructor

962c9f4

This can avoid some copies of the iterators.

romainbrenguier force-pushed the feature/range-drop branch from e6cb484 to 962c9f4 Compare March 8, 2019 14:14

tautschnig approved these changes Mar 8, 2019

View reviewed changes

romainbrenguier merged commit d91b397 into diffblue:develop Mar 8, 2019

romainbrenguier deleted the feature/range-drop branch March 8, 2019 15:34

allredj reviewed Mar 8, 2019

View reviewed changes

Add ranget drop and skip methods #4347

Add ranget drop and skip methods #4347

Uh oh!

Conversation

romainbrenguier commented Mar 7, 2019

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romainbrenguier commented Mar 7, 2019

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

romainbrenguier commented Mar 8, 2019

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!