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

More range functionality, unit tests. #46

Merged
merged 5 commits into from
Oct 28, 2016

Conversation

halfflat
Copy link
Contributor

Part of complex compartments feature update.

  • Tests for math::pi, math::lerp, math::area_frustrum and math::volume_frustrum
  • Fix math:pi<long double>().
  • New filter view: lazily selects based on predicate.
  • Generic front and back for sequences.
  • New range STL wrappers stable_sort_by, all_of, any_of.
  • Consolidate common utility unit testing structures into tests/unit/common.hpp

Julia 0.5 deprecates use of `symbol` instead of
`Symbol`. This patch just substitutes the
correct call.
* Tests for `math::pi`, `math::lerp`, `math::area_frustrum`
  and `math::volume_frustrum`
* Fix `math:pi<long double>()`.
* New `filter` view: lazily selects based on predicate.
* Generic `front` and `back` for sequences.
* New rangeutil STL wrappers `stable_sort_by`, `all_of`, `any_of`.
* Consolidate common utility unit testing structures into
  `tests/unit/common.hpp`
Copy link
Member

@bcumming bcumming left a comment

Choose a reason for hiding this comment

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

The implementation of math::lerp appears to be missing from this PR.

template <typename I, bool = std::is_pointer<I>::value>
struct arrow {
using type = decltype(std::declval<I>().operator->());
static type eval(I& x) { return x.operator->(); }
Copy link
Member

Choose a reason for hiding this comment

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

Could you use a const reference here, because it is return by value?

static type eval(const I& x) { return x.operator->(); }

template <typename I>
struct arrow<I, true> {
using type = I;
static type eval(I& x) { return x; }
Copy link
Member

Choose a reason for hiding this comment

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

ditto

static type eval(I& x) { return x; }
};
}

Copy link
Member

Choose a reason for hiding this comment

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

A comment that briefly describes I, S and F is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add.


public:
using value_type = typename std::iterator_traits<I>::value_type;
using difference_type = typename std::iterator_traits<I>::difference_type;
Copy link
Member

Choose a reason for hiding this comment

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

This is a general question, because I haven't got so much experience implementing iterators. What is the purpose of defining iterator_category here? And why don't we inherit from std::iterator? I saw that this has been depricated in c++17, but I can't find any reasoning about why.

Copy link
Member

Choose a reason for hiding this comment

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

I am sure that you will let me know when you review my iterator implementation in PR #45 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

iterator_category is added so that it can play nice with others; in particular, filter_iterator has useful semantics over a sequence described by input iterators, but then it itself no longer has forward iterator semantics, and this should be signalled to any generic code that uses it.

The committee's justification for the deprecation of std::iterator can be found here,
tl;dr: lacks clarity, and need to import typedefs from base class anyway to use them in-class.

mutable I inner_;
S end_;
mutable bool ok_;
mutable uninitialized<F> f_; // always in initialized state post-construction
Copy link
Member

Choose a reason for hiding this comment

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

A comment on the rationale behind using uninitialized is needed, because it is not immediately obvious why to someone unfamiliar with the library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add (also for transform_iterator).

@halfflat
Copy link
Contributor Author

math::lerp had already snuck into master.

* Add documentation of template parameters for `filter_iterator`.
* Document use of `uninitalized<F>` for holding functional objects
  in `filter_iterator` and `transform_iterator`
@bcumming bcumming merged commit c97135d into arbor-sim:master Oct 28, 2016
@halfflat halfflat deleted the feature/more-range-utils branch October 28, 2016 16:46
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.

None yet

2 participants