Review Request : Boost.Range Extension
Hi, Neil Groves and Boosters. OvenToBoost is a project that intend to add some features of Oven Range Library into Boost.Range. I'd like to request a review. Features: - Additional Range Adaptors (taken, taken_while, dropped, dropped_while, elements, elements_key, memoized, outdirected) - Extensions for using Lambda (regular function, regular operator) - Infinite Range (iteration function) - and additional range utilities. Future Plan: I have written primary features. If these features are accept, I'd like to implement/add following features: - Range of Range Adaptors (concatenated, transposed) - Cyclic Range - Range Utilities for String Source Code: https://github.com/faithandbrave/OvenToBoost Document: http://dl.dropbox.com/u/1682460/git/OvenToBoost/libs/range/doc/html/index.ht... Regards, Akira
======================== Akira Takahashi mailto:faithandbrave@gmail.com site: https://sites.google.com/site/faithandbrave/about/en
Hi Akira, Akira Takahashi wrote:
Hi, Neil Groves and Boosters.
OvenToBoost is a project that intend to add some features of Oven Range Library into Boost.Range.
First, congrats on getting things done! I'm really looking forward to see oven-powered Boost.Range. A few comments: [Infinite ranges] Boost.Iterator now has function_input_iterator which can be used to represent infinite ranges. IMHO, function_input_iterator is not so flexible and its implementation has some flaw: https://svn.boost.org/trac/boost/ticket/5825 Oven's infinite range does not have such an issue, right? I think it's nice to start a new thread to introduce and discuss oven's infinite range. [Incompatibilities between Boost.Range and Oven] Oven has some incompatibilities with Boost.Range. For example, boost::as_literal is not the same as oven::as_literal. It is oven::as_c_str that corresponds to boost::as_literal. To support these oven functions, we need to introduce breaking changes. So I'm also interested in the next version of Boost.Range, which might introduce breaking changes and fully uses C++11 features :) P.S. Boost.Range has recently changed size_type from signed to unsigned. (This is done only on trunk). Does this affect your library? Regards, Michel
Hi Michel, Thanks for comment.
[Infinite ranges] Boost.Iterator now has function_input_iterator which can be used to represent infinite ranges. IMHO, function_input_iterator is not so flexible and its implementation has some flaw: https://svn.boost.org/trac/boost/ticket/5825 Oven's infinite range does not have such an issue, right?
iteration() function is OvenToBoost's infinite range. This function's main purpose is makes number of sequence. But, iteration() function can apply many case with other Range adaptors. Past-the-end condition can specified taken/taken_while (and other adaptors). basic past-the-end sample #include <iostream> #include <boost/range/algorithm/for_each.hpp> #include <boost/range/iteration.hpp> #include <boost/range/adaptor/taken_while.hpp> int next_value(int x) { return x + 1; } bool is_valid(int x) { return x < 10; } void print(int x) { std::cout << x << " "; } int main() { using namespace boost::adaptors; boost::for_each( boost::iteration(0, next_value) | taken_while(is_valid), print); } // output: // 0 1 2 3 4 5 6 7 8 9 #5825 alternative example: #include <string> #include <fstream> #include <iostream> #include <boost/range/algorithm/for_each.hpp> #include <boost/range/iteration.hpp> #include <boost/range/adaptor/taken.hpp> static const std::string filename( "test.txt" ); struct generator { typedef int result_type; generator() : in(0) {} generator(std::ifstream& in) : in(&in) {} result_type operator() (int) { result_type ret; (*in) >> ret; return ret; } std::ifstream* in; }; void print(int x) { std::cout << x << " "; } int main() { std::ofstream out(filename); out << 0 << std::endl << 1 << std::endl << 2 << std::endl << 3 << std::endl; std::ifstream in(filename); generator f(in); boost::for_each(boost::iteration(f(0), f) | boost::adaptors::taken(3), print); } // output: // 0 1 2
[Incompatibilities between Boost.Range and Oven] Oven has some incompatibilities with Boost.Range. For example, boost::as_literal is not the same as oven::as_literal. It is oven::as_c_str that corresponds to boost::as_literal. To support these oven functions, we need to introduce breaking changes.
Now version, OvenToBoost not include as_literal/c_str. I don't intend to introduce breaking change. This project is not full Oven.
Boost.Range has recently changed size_type from signed to unsigned. (This is done only on trunk). Does this affect your library?
No affect. Thanks, Akira
Akira Takahashi wrote:
[Infinite ranges] Boost.Iterator now has function_input_iterator which can be used to represent infinite ranges. IMHO, function_input_iterator is not so flexible and its implementation has some flaw: https://svn.boost.org/trac/boost/ticket/5825 Oven's infinite range does not have such an issue, right?
iteration() function is OvenToBoost's infinite range. This function's main purpose is makes number of sequence. But, iteration() function can apply many case with other Range adaptors. Past-the-end condition can specified taken/taken_while (and other adaptors). <snip>
Thanks for the examples. I would like `nonstop` and `generation` as well to be included in OvenToBoost! One question about implementation of `single`: template <class T> inline iterator_range<const T*> single(const T& x) { return make_iterator_range(boost::addressof(x), boost::addressof(x) + 1); } I'm concerned about taking one-past-the-end pointer of non-array object. Is this guaranteed to work? If so, could you tell me which part of the Standard guarantee it? Regards, Michel
2012/5/31 Michel Morin <mimomorin@gmail.com>:
I'm concerned about taking one-past-the-end pointer of non-array object. Is this guaranteed to work? If so, could you tell me which part of the Standard guarantee it?
standard part is follow: 5.7.4 For the purposes of these operators, a pointer to a nonarray object behaves the same as a pointer to the first element of an array of length one with the type of the object as its element type.
Akira Takahashi wrote:
I'm concerned about taking one-past-the-end pointer of non-array object. Is this guaranteed to work? If so, could you tell me which part of the Standard guarantee it?
standard part is follow:
5.7.4 For the purposes of these operators, a pointer to a nonarray object behaves the same as a pointer to the first element of an array of length one with the type of the object as its element type.
Ah, I missed that paragraph... thanks for your answer! I just applied inspection tool (http://www.boost.org/tools/inspect/index.html). to OvenToBoost (459355e). It's not a thing to fix right now, but anyway here is the inspection result: [boost-root] README: *C*, *Lic* [range] boost/range/access/at.hpp: *Tab* boost/range/access/front.hpp: *Tab* boost/range/adaptor/dropped.hpp: *Tab* boost/range/adaptor/memoized.hpp: (line 78) Unnamed namespace boost/range/algorithm_ext/all_of.hpp: *Tab* boost/range/algorithm_ext/any_of.hpp: *Tab* boost/range/algorithm_ext/none_of.hpp: *Tab* boost/range/as_container.hpp: (line 50) Unnamed namespace boost/range/irange_ex.hpp: (line 35) Unnamed namespace libs/range/test/access_test/at.cpp: *Tab* libs/range/test/adaptor_test/memoized.cpp: *Tab* libs/range/test/directory_range_test_dir/a.txt: *C*, *Lic* libs/range/test/directory_range_test_dir/c_dir/d.cpp: *C*, *END* file doesn't end with a newline, *Lic* Details *Lic* missing Boost license info, or wrong reference text *C* missing copyright notice *EOL* invalid (cr only) line-ending *END* file doesn't end with a newline *LINK* invalid bookmarks, duplicate bookmarks, invalid urls, broken links, unlinked files *N* file and directory name issues *Tab* tabs in file *ASCII* non-ASCII chars in file *APPLE-MACROS* calls to Apple's debugging macros in file *ASSERT-MACROS* presence of C-style assert macro in file (use BOOST_ASSERT instead) *M* uses of min or max that have not been protected from the min/max macros, or unallowed #undef-s *U* unnamed namespace in header Directories with a file named "boost-no-inspect" will not be inspected. Files containing "boost-no-inspect" will not be inspected. Hope this helps, Michel
2012/5/31 Michel Morin <mimomorin@gmail.com>:
I just applied inspection tool (http://www.boost.org/tools/inspect/index.html). to OvenToBoost (459355e). It's not a thing to fix right now, but anyway here is the inspection result:
[boost-root] README: *C*, *Lic*
[range] boost/range/access/at.hpp: *Tab* boost/range/access/front.hpp: *Tab* boost/range/adaptor/dropped.hpp: *Tab* boost/range/adaptor/memoized.hpp: (line 78) Unnamed namespace boost/range/algorithm_ext/all_of.hpp: *Tab* boost/range/algorithm_ext/any_of.hpp: *Tab* boost/range/algorithm_ext/none_of.hpp: *Tab* boost/range/as_container.hpp: (line 50) Unnamed namespace boost/range/irange_ex.hpp: (line 35) Unnamed namespace libs/range/test/access_test/at.cpp: *Tab* libs/range/test/adaptor_test/memoized.cpp: *Tab* libs/range/test/directory_range_test_dir/a.txt: *C*, *Lic* libs/range/test/directory_range_test_dir/c_dir/d.cpp: *C*, *END* file doesn't end with a newline, *Lic*
Details *Lic* missing Boost license info, or wrong reference text *C* missing copyright notice *EOL* invalid (cr only) line-ending *END* file doesn't end with a newline *LINK* invalid bookmarks, duplicate bookmarks, invalid urls, broken links, unlinked files *N* file and directory name issues *Tab* tabs in file *ASCII* non-ASCII chars in file *APPLE-MACROS* calls to Apple's debugging macros in file *ASSERT-MACROS* presence of C-style assert macro in file (use BOOST_ASSERT instead) *M* uses of min or max that have not been protected from the min/max macros, or unallowed #undef-s *U* unnamed namespace in header Directories with a file named "boost-no-inspect" will not be inspected. Files containing "boost-no-inspect" will not be inspected.
Thanks! I fixed licence, copyright, and tab.
Hi Akira, On Tue, May 29, 2012 at 7:50 AM, Akira Takahashi <faithandbrave@gmail.com>wrote:
Hi, Neil Groves and Boosters.
OvenToBoost is a project that intend to add some features of Oven Range Library into Boost.Range.
I'd like to request a review.
Features: - Additional Range Adaptors (taken, taken_while, dropped, dropped_while, elements, elements_key, memoized, outdirected) - Extensions for using Lambda (regular function, regular operator) - Infinite Range (iteration function) - and additional range utilities.
Looks good. I certainly would be happy to moderate and include the approved features into Boost.Range. I should be able to devote more time to maintaining and improving Boost.Range from now on.
Future Plan: I have written primary features. If these features are accept, I'd like to implement/add following features:
- Range of Range Adaptors (concatenated, transposed) - Cyclic Range - Range Utilities for String
I have quite a list of features to add too, we should discuss these but let's make sure we clearly focus upon the stuff that is ready first to give it the best chance for making it into a release soon.
Source Code: https://github.com/faithandbrave/OvenToBoost
Document:
http://dl.dropbox.com/u/1682460/git/OvenToBoost/libs/range/doc/html/index.ht...
Regards, Akira
This looks really exciting. I can't wait to make some big improvements again to Boost.Range. I promise to do a proper informal review first and try and help with the review process.
======================== Akira Takahashi mailto:faithandbrave@gmail.com site: https://sites.google.com/site/faithandbrave/about/en
Regards, Neil Groves
Hi Neil, thanks for your reply.
This looks really exciting. I can't wait to make some big improvements again to Boost.Range. I promise to do a proper informal review first and try and help with the review process.
I will devote a full effort to make Boost.Range better, too! Thanks, Akira
Hi Akira and Neil, I will add Boost.Range extensions to the review schedule with Neil as review manager. Best, Ron On May 29, 2012, at 4:49 PM, Neil Groves wrote:
Hi Akira,
On Tue, May 29, 2012 at 7:50 AM, Akira Takahashi <faithandbrave@gmail.com>wrote:
Hi, Neil Groves and Boosters.
OvenToBoost is a project that intend to add some features of Oven Range Library into Boost.Range.
I'd like to request a review.
Features: - Additional Range Adaptors (taken, taken_while, dropped, dropped_while, elements, elements_key, memoized, outdirected) - Extensions for using Lambda (regular function, regular operator) - Infinite Range (iteration function) - and additional range utilities.
Looks good. I certainly would be happy to moderate and include the approved features into Boost.Range. I should be able to devote more time to maintaining and improving Boost.Range from now on.
Future Plan: I have written primary features. If these features are accept, I'd like to implement/add following features:
- Range of Range Adaptors (concatenated, transposed) - Cyclic Range - Range Utilities for String
I have quite a list of features to add too, we should discuss these but let's make sure we clearly focus upon the stuff that is ready first to give it the best chance for making it into a release soon.
Source Code: https://github.com/faithandbrave/OvenToBoost
Document:
http://dl.dropbox.com/u/1682460/git/OvenToBoost/libs/range/doc/html/index.ht...
Regards, Akira
This looks really exciting. I can't wait to make some big improvements again to Boost.Range. I promise to do a proper informal review first and try and help with the review process.
======================== Akira Takahashi mailto:faithandbrave@gmail.com site: https://sites.google.com/site/faithandbrave/about/en
Regards, Neil Groves
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Akira Takahashi wrote:
Features: <snip> - Extensions for using Lambda (regular function, regular operator) <snip>
I took a look at the implementation of `regular` and found that it is implemented using `shared_pointer`. What are the pros and cons of the `shared_pointer`-based implementation compared to an obvious `optional`-based implementation? Regards, Michel
2012/6/1 Michel Morin <mimomorin@gmail.com>:
Akira Takahashi wrote:
Features: <snip> - Extensions for using Lambda (regular function, regular operator) <snip>
I took a look at the implementation of `regular` and found that it is implemented using `shared_pointer`.
What are the pros and cons of the `shared_pointer`-based implementation compared to an obvious `optional`-based implementation?
This is mistake implementation. I fixed implementation, boost::shared_ptr to boost::optional.
Akira Takahashi wrote:
I took a look at the implementation of `regular` and found that it is implemented using `shared_pointer`.
What are the pros and cons of the `shared_pointer`-based implementation compared to an obvious `optional`-based implementation?
This is mistake implementation. I fixed implementation, boost::shared_ptr to boost::optional.
IIUC, the output of `regular` does not satisfy the Regular concept. Is this just an implementation deficiency or intentional behavior? If it is intentional, then the name `regular` might not be a good name... Regards, Michel
2012/6/2 Michel Morin <mimomorin@gmail.com>:
Akira Takahashi wrote: IIUC, the output of `regular` does not satisfy the Regular concept. Is this just an implementation deficiency or intentional behavior? If it is intentional, then the name `regular` might not be a good name...
Now fixing missing feature of Regular concept.
Akira Takahashi wrote:
IIUC, the output of `regular` does not satisfy the Regular concept. Is this just an implementation deficiency or intentional behavior? If it is intentional, then the name `regular` might not be a good name...
Now fixing missing feature of Regular concept.
Nice :) A few comments:
template <class F> RegularFunctorType regular(F f)
* What is the requirement of `F`? CopyConstructible? This should be documented, too. * If `F` has relational operators, do `RegularFunctorType`'s relational operators use them? If not, I think it would be better to document it. * `indirect_functor` does not support `boost::result_of` protocol for nullary function calls. See ticket #6914 for details https://svn.boost.org/trac/boost/ticket/6914 Regards, Michel
`as_container` looks interesting! Here are some comments on `as_container`: * `as_container_functor` and `operator |` live in different name spaces. So ADL does not work. * `as_container` should live in boost::adaptors? * Built-in arrays are not supported by `as_container`. * Template conversion operator ambiguity: In C++03 and C++11, the following code boost::array<int, 3> ar = {{1, 2, 3}}; std::vector<int> v(ar | as_container); fails to compile due to the ambiguity between - `explicit vector(const allocator_type&)`, - `vector(initializer_list<value_type>)`, and - `vector(const vector&)`. In C++11, the following code boost::array<int, 3> ar = {{1, 2, 3}}; std::vector<int> v; v = ar | as_container; fails to compile due to the ambiguity between - `vector& operator=(initializer_list<value_type>)`, and - `vector& operator=(const vector&)`. One solution to this problem is applying SFINAE by "default template arguments for function templates" to the template conversion operator: template < class Container , class Dummy1 = typename Container::difference_type , class Dummy2 = typename Container::iterator > operator Container() const { /* ... */ } (`Dummy1` is used to SFINAE out `std::initializer_list` and `Dummy2` is used to SFINAE out `std::allocator`.) But this is a C++11 feature and so ambiguity with `std::allocator` is not solved in C++03. If there is no simple solution, simply documenting the limitation is an acceptable solution, I think. Regards, Michel
2012/6/7 Michel Morin <mimomorin@gmail.com>:
`as_container` looks interesting!
`as_container` is experimental feature. not include primary release. (moved to experimental directory.) I will add move_iterator version. dest = src | as_moved_container; ( Container(make_move_iterator(begin(src), make_move_iterator(end(src)); )
Here are some comments on `as_container`: * `as_container_functor` and `operator |` live in different name spaces. So ADL does not work.
* `as_container` should live in boost::adaptors?
`as_container` is different other range adaptors. `oven::copied` categorized to range algorithm, not range adaptor.
* Built-in arrays are not supported by `as_container`.
* Template conversion operator ambiguity: In C++03 and C++11, the following code boost::array<int, 3> ar = {{1, 2, 3}}; std::vector<int> v(ar | as_container); fails to compile due to the ambiguity between - `explicit vector(const allocator_type&)`, - `vector(initializer_list<value_type>)`, and - `vector(const vector&)`.
In C++11, the following code boost::array<int, 3> ar = {{1, 2, 3}}; std::vector<int> v; v = ar | as_container; fails to compile due to the ambiguity between - `vector& operator=(initializer_list<value_type>)`, and - `vector& operator=(const vector&)`.
One solution to this problem is applying SFINAE by "default template arguments for function templates" to the template conversion operator: template < class Container , class Dummy1 = typename Container::difference_type , class Dummy2 = typename Container::iterator > operator Container() const { /* ... */ } (`Dummy1` is used to SFINAE out `std::initializer_list` and `Dummy2` is used to SFINAE out `std::allocator`.) But this is a C++11 feature and so ambiguity with `std::allocator` is not solved in C++03. If there is no simple solution, simply documenting the limitation is an acceptable solution, I think.
Interesting. I think better way is disable_if<!is_initializer_list<Container>>. I will solve later.
Akira Takahashi wrote:
`as_container` looks interesting!
`as_container` is experimental feature. not include primary release. (moved to experimental directory.)
OK, so it is undocumented :)
* Template conversion operator ambiguity: In C++03 and C++11, the following code boost::array<int, 3> ar = {{1, 2, 3}}; std::vector<int> v(ar | as_container); fails to compile...
In C++11, the following code boost::array<int, 3> ar = {{1, 2, 3}}; std::vector<int> v; v = ar | as_container; fails to compile... <snip> Interesting. I think better way is disable_if<!is_initializer_list<Container>>. I will solve later.
#if !defined(BOOST_NO_INITIALIZER_LISTS) && !defined(BOOST_NO_FUNCTION_TEMPLATE_DEFAULT_ARGS)
`BOOST_NO_INITIALIZER_LISTS` is decpreated. Use `BOOST_NO_CXX11_HDR_INITIALIZER_LIST` instead. Please consult to the doc (in trunk) of Boost.Config. Regards, Michel
2012/6/6 Michel Morin <mimomorin@gmail.com>:
Now fixing missing feature of Regular concept.
Nice :)
A few comments:
template <class F> RegularFunctorType regular(F f)
* What is the requirement of `F`? CopyConstructible? This should be documented, too.
* If `F` has relational operators, do `RegularFunctorType`'s relational operators use them? If not, I think it would be better to document it.
documented!
* `indirect_functor` does not support `boost::result_of` protocol for nullary function calls. See ticket #6914 for details https://svn.boost.org/trac/boost/ticket/6914
fixed. and add test code.
Akira Takahashi wrote:
* `indirect_functor` does not support `boost::result_of` protocol for nullary function calls. See ticket #6914 for details https://svn.boost.org/trac/boost/ticket/6914
fixed. and add test code.
Specialization of `const indirect_functor` is missing. Regards, Michel
Michel Morin wrote:
Akira Takahashi wrote:
* `indirect_functor` does not support `boost::result_of` protocol for nullary function calls. See ticket #6914 for details https://svn.boost.org/trac/boost/ticket/6914
fixed. and add test code.
Specialization of `const indirect_functor` is missing.
Oops. It's already added. I wrongly tested using *reference* to `const indirect_functor`... Regards, Michel
Just to be clear. Michel Morin wrote:
* `indirect_functor` does not support `boost::result_of` protocol for nullary function calls. See ticket #6914 for details https://svn.boost.org/trac/boost/ticket/6914
fixed. and add test code.
Specialization of `const indirect_functor` is missing.
Please ignore this comment. This comment was wrong and your code is OK. Sorry for the noise. Regards, Michel
Akira Takahashi wrote:
Source Code: https://github.com/faithandbrave/OvenToBoost
Small nitpicking: [adaptor/dropped_while.hpp] * Don't forget `#pragma warning(pop)`. [as_container.hpp] * Missing include guard. [detail/indirect_functor.hpp] * Use `BOOST_NO_XXXX` to detect C++11 features (`BOOST_HAS_XXXX` is deprecated). * Even if a compiler supports C++11, the availability of `std::forward` in the standard library is not guaranteed. Use `static_cast` as substitute of `std::forward`. (Or just abandon such a pathological case and use `std::forward`.) Regards, Michel
2012/6/1 Michel Morin <mimomorin@gmail.com>:
Akira Takahashi wrote:
Source Code: https://github.com/faithandbrave/OvenToBoost
Small nitpicking:
[adaptor/dropped_while.hpp] * Don't forget `#pragma warning(pop)`.
[as_container.hpp] * Missing include guard.
[detail/indirect_functor.hpp] * Use `BOOST_NO_XXXX` to detect C++11 features (`BOOST_HAS_XXXX` is deprecated). * Even if a compiler supports C++11, the availability of `std::forward` in the standard library is not guaranteed. Use `static_cast` as substitute of `std::forward`. (Or just abandon such a pathological case and use `std::forward`.)
Thanks for many review. Fixed!
Akira Takahashi wrote:
Document: http://dl.dropbox.com/u/1682460/git/OvenToBoost/libs/range/doc/html/index.ht...
Reference section does not contain `regular` function template, but it should. There, I'd like to know the preconditions of the input and the concepts satisfied by the output. Regards, Michel
participants (4)
-
Akira Takahashi -
Michel Morin -
Neil Groves -
Ronald Garcia