[review][STLInterfaces] STLInterfaces review results

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[review][STLInterfaces] STLInterfaces review results

Boost - Dev mailing list
Many thanks are owed to all who participated in the formal review of Zach
Laine's STLInterfaces library, especially during this busy holiday season:

* Hadriel Kaplan - ACCEPT
* Andreas Wass - ACCEPT
* Rainer Deyke - ACCEPT
* Arthur Gruzauskas - ACCEPT
* Krystian Stasiowski - ACCEPT
* Daniela Engert - CONDITIONALLY ACCEPT
* Gavin Lambert - Technical comments

First, a summary of the reviews:

1. Design
Reviewers were unanimous in their appreciation for the design. Krystian had
the only asterisk about the design, noting that derived class members
should ideally be SFINAE-detected, where the library feature would SFINAE
where the corresponding required derived feature detection fails. I have a
different opinion about this, but we can discuss that elsewhere.

2. Implementation
The container_interface destructor and the optional dependency on cmcstl2
were snags for multiple reviewers. Several compile errors were discovered
that need to be addressed.

3. Documentation
Reviewers found the documentation to be "ok". A common request was for
better organization of derived-class requirements, particularly with the
container_interface template. Reviewers also lamented the lack of training
wheels for the subject matter.

4. Tests
Hadriel noted the lack of non-copyable value_type tests, while Daniela
noted the lack of Boost.Build support. Hadriel was the only one who
commented on the quality of tests ("good"), and the only one who ran them.
Arthur did his own testing, to satisfaction. I appreciate the presence of
SFINAE-friendliness tests and the continuous integration.

5. Usefulness
Reviewers appreciated the usefulness of the library. Arthur is "using it
happily", while others found it "very useful", "fairly useful", or of
"high" usefulness. However, Krystian's response implied that this library
may not be as useful for containers needing extreme performance.

The reviews clearly demonstrate a consensus. Accordingly, I'm pleased to
announce that STLInterfaces is CONDITIONALLY ACCEPTED as a Boost library.

Conditions for acceptance:
1. Remove optional dependency on cmcstl2 (issue #27)
2. Add Boost license comments to all files, including appveyor.yml,
.travis.yml, README.md, */CMakeLists.txt (issue #26)
3. Support Boost.Build (issue #25)
4. Replace googletest with Boost equivalent (issue #24)
5. Remove usage of non-existent v2_dtl members in container_interface.hpp
(issue #22)
6. Include <tuple> for std::tie usage in C++14 mode (issue #21)
7. Fix dangling parens in container_inteface.hpp:731 (issue #20)
8. Make Concepts usage C++20-conformant (issue #13)
9. Remove the destructor in container_interface (issue #12)
10. Add tests for non-copyable value types (issue #30)

Suggestions:
1. Explicitly document the signatures and return types of functions
provided by base class templates (issue #23)
2. Mention the C++20 generator pattern in documentation (issue #19)
3. Document the library's interactions with Ranges (issue #18)
4. Remove unnecessary comments (issue #17)
5. Investigate compile error in VS2015 (msvc 19.0.3) due to issues with
noexcept() operator in function overloading (issue #16)
6. Rename container_interface to sequence_container_interface (issue #15)
7. Change the 'contiguous' bool to an enum (issue #14)
8. Document the relationship between value_type operator== and
containter_interface-defined operator== (issue #11)
9. Improve table/section names for better navigation of container_interface
tutorial (issue #10)
10. Clean up BOOST_STL_INTERFACES_DOXYGEN usage (issue #28)
11. Split up fwd.hpp into multiple headers (issue #29)
12. Document optimization trade-offs of proxy_iterator_interface (issue #30)

Rationale:
1. Condition #1 is, I feel, the appropriate default as review manager. If
Zach feels strongly about keeping the optional dependency on cmcstl2, we
should discuss.
2. Unless the library requirements webpage has fallen out of date, new
libraries must still support Boost.Build.
3. A not-so-quick grep of current Boost libraries yields no `gtest`
results. I assume that we do not allow googletest as the sole test
framework in a library.
4. The other conditions should be self-explanatory.

Thank you Zach for your contributions, and I look forward to working with
you to add this library into Boost.

Regards,
Barrett Adair

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [review][STLInterfaces] STLInterfaces review results

Boost - Dev mailing list
On Thu, Dec 26, 2019 at 2:31 AM Barrett Adair via Boost <
[hidden email]> wrote:

> Many thanks are owed to all who participated in the formal review of Zach
> Laine's STLInterfaces library, especially during this busy holiday season:
>
> * Hadriel Kaplan - ACCEPT
> * Andreas Wass - ACCEPT
> * Rainer Deyke - ACCEPT
> * Arthur Gruzauskas - ACCEPT
> * Krystian Stasiowski - ACCEPT
> * Daniela Engert - CONDITIONALLY ACCEPT
> * Gavin Lambert - Technical comments
>
> First, a summary of the reviews:
>
> 1. Design
> Reviewers were unanimous in their appreciation for the design. Krystian had
> the only asterisk about the design, noting that derived class members
> should ideally be SFINAE-detected, where the library feature would SFINAE
> where the corresponding required derived feature detection fails. I have a
> different opinion about this, but we can discuss that elsewhere.
>
> 2. Implementation
> The container_interface destructor and the optional dependency on cmcstl2
> were snags for multiple reviewers. Several compile errors were discovered
> that need to be addressed.
>
> 3. Documentation
> Reviewers found the documentation to be "ok". A common request was for
> better organization of derived-class requirements, particularly with the
> container_interface template. Reviewers also lamented the lack of training
> wheels for the subject matter.
>
> 4. Tests
> Hadriel noted the lack of non-copyable value_type tests, while Daniela
> noted the lack of Boost.Build support. Hadriel was the only one who
> commented on the quality of tests ("good"), and the only one who ran them.
> Arthur did his own testing, to satisfaction. I appreciate the presence of
> SFINAE-friendliness tests and the continuous integration.
>
> 5. Usefulness
> Reviewers appreciated the usefulness of the library. Arthur is "using it
> happily", while others found it "very useful", "fairly useful", or of
> "high" usefulness. However, Krystian's response implied that this library
> may not be as useful for containers needing extreme performance.
>
> The reviews clearly demonstrate a consensus. Accordingly, I'm pleased to
> announce that STLInterfaces is CONDITIONALLY ACCEPTED as a Boost library.
>
> Conditions for acceptance:
> 1. Remove optional dependency on cmcstl2 (issue #27)
> 2. Add Boost license comments to all files, including appveyor.yml,
> .travis.yml, README.md, */CMakeLists.txt (issue #26)
> 3. Support Boost.Build (issue #25)
> 4. Replace googletest with Boost equivalent (issue #24)
> 5. Remove usage of non-existent v2_dtl members in container_interface.hpp
> (issue #22)
> 6. Include <tuple> for std::tie usage in C++14 mode (issue #21)
> 7. Fix dangling parens in container_inteface.hpp:731 (issue #20)
> 8. Make Concepts usage C++20-conformant (issue #13)
> 9. Remove the destructor in container_interface (issue #12)
> 10. Add tests for non-copyable value types (issue #30)
>
> Suggestions:
> 1. Explicitly document the signatures and return types of functions
> provided by base class templates (issue #23)
> 2. Mention the C++20 generator pattern in documentation (issue #19)
> 3. Document the library's interactions with Ranges (issue #18)
> 4. Remove unnecessary comments (issue #17)
> 5. Investigate compile error in VS2015 (msvc 19.0.3) due to issues with
> noexcept() operator in function overloading (issue #16)
> 6. Rename container_interface to sequence_container_interface (issue #15)
> 7. Change the 'contiguous' bool to an enum (issue #14)
> 8. Document the relationship between value_type operator== and
> containter_interface-defined operator== (issue #11)
> 9. Improve table/section names for better navigation of container_interface
> tutorial (issue #10)
> 10. Clean up BOOST_STL_INTERFACES_DOXYGEN usage (issue #28)
> 11. Split up fwd.hpp into multiple headers (issue #29)
> 12. Document optimization trade-offs of proxy_iterator_interface (issue
> #30)
>
> Rationale:
> 1. Condition #1 is, I feel, the appropriate default as review manager. If
> Zach feels strongly about keeping the optional dependency on cmcstl2, we
> should discuss.
> 2. Unless the library requirements webpage has fallen out of date, new
> libraries must still support Boost.Build.
> 3. A not-so-quick grep of current Boost libraries yields no `gtest`
> results. I assume that we do not allow googletest as the sole test
> framework in a library.
> 4. The other conditions should be self-explanatory.
>
> Thank you Zach for your contributions, and I look forward to working with
> you to add this library into Boost.
>
> Regards,
> Barrett Adair
>
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost
>

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [review][STLInterfaces] STLInterfaces review results

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Thu, Dec 26, 2019 at 2:31 AM Barrett Adair via Boost <
[hidden email]> wrote:

> Many thanks are owed to all who participated in the formal review of Zach
> Laine's STLInterfaces library, especially during this busy holiday season:
>
> * Hadriel Kaplan - ACCEPT
> * Andreas Wass - ACCEPT
> * Rainer Deyke - ACCEPT
> * Arthur Gruzauskas - ACCEPT
> * Krystian Stasiowski - ACCEPT
> * Daniela Engert - CONDITIONALLY ACCEPT
> * Gavin Lambert - Technical comments
>

Thanks to everyone who participated in the review!

3. Documentation
> Reviewers found the documentation to be "ok". A common request was for
> better organization of derived-class requirements, particularly with the
> container_interface template. Reviewers also lamented the lack of training
> wheels for the subject matter.
>

I already have a Github ticket to add better organization to what already
exists in the container_interface docs, and I agree that more up-front
basic introduction is probably useful for those new to the domain.


> 4. Tests
> Hadriel noted the lack of non-copyable value_type tests,


This is a good point.  I'll add these.


> The reviews clearly demonstrate a consensus. Accordingly, I'm pleased to
> announce that STLInterfaces is CONDITIONALLY ACCEPTED as a Boost library.
>
> Conditions for acceptance:
> 1. Remove optional dependency on cmcstl2 (issue #27)
> 2. Add Boost license comments to all files, including appveyor.yml,
> .travis.yml, README.md, */CMakeLists.txt (issue #26)
> 3. Support Boost.Build (issue #25)
> 4. Replace googletest with Boost equivalent (issue #24)
> 5. Remove usage of non-existent v2_dtl members in container_interface.hpp
> (issue #22)
> 6. Include <tuple> for std::tie usage in C++14 mode (issue #21)
> 7. Fix dangling parens in container_inteface.hpp:731 (issue #20)
> 8. Make Concepts usage C++20-conformant (issue #13)
> 9. Remove the destructor in container_interface (issue #12)
> 10. Add tests for non-copyable value types (issue #30)
>

I'll get to work on these.  FWIW, I always intended to switch to
Boost.Build and Boost.Test after the review -- I just failed to mention
this beforehand.  As for the use of cmcstl2, I think it was important to
get this working to prove to myself that the C++20 version can be
implemented without several of the limitations that the pre-concepts
version suffers from.


> Suggestions:
> 1. Explicitly document the signatures and return types of functions
> provided by base class templates (issue #23)
> 2. Mention the C++20 generator pattern in documentation (issue #19)
> 3. Document the library's interactions with Ranges (issue #18)
> 4. Remove unnecessary comments (issue #17)
> 5. Investigate compile error in VS2015 (msvc 19.0.3) due to issues with
> noexcept() operator in function overloading (issue #16)
> 6. Rename container_interface to sequence_container_interface (issue #15)
> 7. Change the 'contiguous' bool to an enum (issue #14)
> 8. Document the relationship between value_type operator== and
> containter_interface-defined operator== (issue #11)
> 9. Improve table/section names for better navigation of container_interface
> tutorial (issue #10)
> 10. Clean up BOOST_STL_INTERFACES_DOXYGEN usage (issue #28)
> 11. Split up fwd.hpp into multiple headers (issue #29)
> 12. Document optimization trade-offs of proxy_iterator_interface (issue
> #30)
>
> Rationale:
> 1. Condition #1 is, I feel, the appropriate default as review manager. If
> Zach feels strongly about keeping the optional dependency on cmcstl2, we
> should discuss.
>

I put it in a v2 namespace, and explicitly called it experimental and
temporary in the docs.  That being said, that clearly is not sufficient to
overcome the large amount of confusion it introduces.  I'll leave it on a
branch on Github, but will not include it in a Boost release.  The v2
implementation that uses the C++20 standard library will re-appear when at
least one major implementation can support it.


> 2. Unless the library requirements webpage has fallen out of date, new
> libraries must still support Boost.Build.
>

Agreed.


> 3. A not-so-quick grep of current Boost libraries yields no `gtest`
> results. I assume that we do not allow googletest as the sole test
> framework in a library.
>

Agreed.


> 4. The other conditions should be self-explanatory.
>

They are.


> Thank you Zach for your contributions, and I look forward to working with
> you to add this library into Boost.
>

Thanks very much Barrett for managing the review!  Having done it once
before, I understand just how much work it is.

Zach

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost