Re: [boost] [review] **NEXT WEEK** Review of Outcome (starts Fri-19-May)
On 05/11/2017 06:19 PM, charleyb123 . via Boost wrote:
The code has a "boost-lite" submodule. Is that part of the review?
The code has a "boost-lite" submodule. Is that part of the review?
In terms of public API, only the single part of boost-lite's API exposed in Outcome's public API which is boost-lite::tribool, which I exposed specially into the reference docs at https://ned14.github.io/boost.outcome/group__tribool.html (Outcome provides ternary based logic as an extension. If the peer review desires, we can fake hide tribool into namespace boost::outcome) Boost-lite provides the cmake based build, config, test, Boost.Test alternative, install, partial preprocess single header include generation and much of the internal implemention classes used by Outcome. However if accepted then Outcome would gain a Boost.Build veneer and be auto generated by script from the standalone Outcome same as Boost.ASIO is, and so I figure none of that is in scope for this review. (that said, constructive comments, especially bug reports, in the out of scope stuff are welcome. There are likely to be a ton of them) Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On Sun, May 14, 2017 at 7:44 AM, Niall Douglas wrote:
The code has a "boost-lite" submodule. Is that part of the review?
However if accepted then Outcome would gain a Boost.Build veneer and be auto generated by script from the standalone Outcome same as Boost.ASIO is, and so I figure none of that is in scope for this review.
Any part of the implementation is in scope for the review, and if "Boost-lite" is not an already reviewed and accepted Boost library, then aren't reviewers allowed to review its implementation (since it is effectively part of the submission)? Glen
On 14/05/2017 12:54, Glen Fernandes wrote:
On Sun, May 14, 2017 at 7:44 AM, Niall Douglas wrote:
The code has a "boost-lite" submodule. Is that part of the review?
However if accepted then Outcome would gain a Boost.Build veneer and be auto generated by script from the standalone Outcome same as Boost.ASIO is, and so I figure none of that is in scope for this review.
Any part of the implementation is in scope for the review, and if "Boost-lite" is not an already reviewed and accepted Boost library, then aren't reviewers allowed to review its implementation (since it is effectively part of the submission)?
About a year ago when I was considering whether to invest the considerable effort to bring Outcome into a Boost ready form I asked here about internal sublibraries. Apparently some years ago they were common enough in submitted new Boost libraries, and indeed some internal sublibraries went on to later become Boost libraries in their own right. We should proceed here with the Outcome review the same way as it was with those preceding cases. Whatever the precedent is. (I'm not sure what the exact precedent is, whomever knows for sure please speak, but be aware that most of the dependency on boost-lite is substitutable for Boost proper) Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On Sun, May 14, 2017 at 10:56 AM, Niall Douglas wrote:
On 14/05/2017 12:54, Glen Fernandes wrote:
On Sun, May 14, 2017 at 7:44 AM, Niall Douglas wrote:
The code has a "boost-lite" submodule. Is that part of the review?
However if accepted then Outcome would gain a Boost.Build veneer and be auto generated by script from the standalone Outcome same as Boost.ASIO is, and so I figure none of that is in scope for this review.
Any part of the implementation is in scope for the review, and if "Boost-lite" is not an already reviewed and accepted Boost library, then aren't reviewers allowed to review its implementation (since it is effectively part of the submission)?
About a year ago when I was considering whether to invest the considerable effort to bring Outcome into a Boost ready form I asked here about internal sublibraries.
Apparently some years ago they were common enough in submitted new Boost libraries, and indeed some internal sublibraries went on to later become Boost libraries in their own right.
We should proceed here with the Outcome review the same way as it was with those preceding cases. Whatever the precedent is.
(I'm not sure what the exact precedent is, whomever knows for sure please speak, but be aware that most of the dependency on boost-lite is substitutable for Boost proper)
I'm not sure I understand what you mean. You're submitting a library for review. Everything in the library is subject to review - the design, the implementation, the documentation, the tests. The implementation of Outcome includes something called boost-lite right now. Reviewers are allowed to review any files in this boost-lite, just as they review any implementation detail of Outcome, right? i.e. There's no difference to them between reviewing boost::outcome::x, boost::outcome::detail::y, boost_lite::z, boost_lite::detail::t If so, we're on the same page. We're on different pages if you're asserting that reviewers not be allowed to review boost_lite::z or boost_lite::detail::t in order to determine whether Outcome has an acceptable implementation for Boost. Glen
About a year ago when I was considering whether to invest the considerable effort to bring Outcome into a Boost ready form I asked here about internal sublibraries.
Apparently some years ago they were common enough in submitted new Boost libraries, and indeed some internal sublibraries went on to later become Boost libraries in their own right.
We should proceed here with the Outcome review the same way as it was with those preceding cases. Whatever the precedent is.
(I'm not sure what the exact precedent is, whomever knows for sure please speak, but be aware that most of the dependency on boost-lite is substitutable for Boost proper)
I'm not sure I understand what you mean. You're submitting a library for review. Everything in the library is subject to review - the design, the implementation, the documentation, the tests.
The implementation of Outcome includes something called boost-lite right now. Reviewers are allowed to review any files in this boost-lite, just as they review any implementation detail of Outcome, right?
It's up to the reviewer as to how much to review or not. Whatever constructive feedback they have, irrespective of whether Outcome uses that part or not, I'll gladly take it. I would say it may make Charley's life tough to disambiguate feedback though. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 5/14/17 7:56 AM, Niall Douglas via Boost wrote:
Apparently some years ago they were common enough in submitted new Boost libraries, and indeed some internal sublibraries went on to later become Boost libraries in their own right.
We should proceed here with the Outcome review the same way as it was with those preceding cases. Whatever the precedent is.
(I'm not sure what the exact precedent is, whomever knows for sure please speak, but be aware that most of the dependency on boost-lite is substitutable for Boost proper)
I can't say I know for sure, but I do have some experience with this. While creating the serialization library, I depended on existing boost solutions to the extent possible. MPL, config, spirit and I'm sure a bunch of others I don't remember. Someone else later added file_system and system to the tests. So there is, was and always will be a strong dependency between the serialization library and the rest of boost. Still I needed some stuff that didn't exist: extended_type_info, composable dataflow iterators, BOOST_STRONG_TYPEDEF, singleton, state_saver, BOOST_STATIC_WARNING. I created them as "private libraries" in order to keep the development effort within bounds. It's much easier to build and maintain a library as the composite of 10 smaller otherwise decoupled components than try to understand things as a whole. I also looked forward when some of these "private libraries" might get promoted to being boost libraries in their own right. What actually happened was: a) I tried to put these in boost namespace as they were not really part of the serialization library itself. I caught hell for this from an important booster who is now no longer involved in boost. So I moved them below the serialization namespace. b) a number of these "private libraries" got adopted for general usage such that I get bug reports related to usage outside the serialization library. These include dataflow iterators, strong typedef, and singleton. c) a number just didn't work out - BOOST_STATIC_WARNING could never really be made to work across all platforms. BOOST_STRONG_TYPEDEF fell out of use in the serialization library due to evolution in understanding on my part - but continues to be used. d) a number got supplanted by real boost libraries - though I didn't update the serialization library to use them. Type index is similar to extended type info. We now have a component for dealing with unicode (which hasn't worked for me BTW). We now have boost.ranges which looks to me to replace dataflow iterators One key component doesn't fit the above - boost::utf8_codecvt written by ron garcia many, many years ago. He must have done a good job as it seems to be the only reliable implementation of this much needed facility. This was one of the components I wanted to add to the boost root but was prohibited from doing so. It was shared between several libraries so it wasn't a good idea to put it under serialization. But being in boost detail there was no place for tests - which was pretty important since may standard libraries had quirky behavior in this area. While no one was looking, I snuck in a test somewhere which helped but we're still #including the source into other libraries because utf8_codecvt was never "officially reviewed". This will be an ongoing saga since <codecvt> is now being dropped from the standard for being unreliable - oh well. So that short story describes my experience in this area - I don't know if it explains anything though. Make of it what you will. Robert Ramey
Niall Douglas wrote:
The code has a "boost-lite" submodule. Is that part of the review?
In terms of public API, only the single part of boost-lite's API exposed in Outcome's public API which is boost-lite::tribool, which I exposed specially into the reference docs at https://ned14.github.io/boost.outcome/group__tribool.html
(Outcome provides ternary based logic as an extension. If the peer review desires, we can fake hide tribool into namespace boost::outcome)
Boost-lite provides the cmake based build, config, test, Boost.Test alternative, install, partial preprocess single header include generation and much of the internal implemention classes used by Outcome. However if accepted then Outcome would gain a Boost.Build veneer and be auto generated by script from the standalone Outcome same as Boost.ASIO is, and so I figure none of that is in scope for this review.
This doesn't exactly inspire confidence. When a library enters Boost and is distributed in Boost releases, there's no longer any point of it duplicating Boost functionality under the hood (without a very good reason); while not requiring Boost is understandable for standalone use, in a Boost release Boost is by definition already available. In addition,
then Outcome would [...] be auto generated by script from the standalone Outcome same as Boost.ASIO is
carries the unfortunate implication that Outcome in Boost may be, similarly to Boost.ASIO, a second-class citizen that may lag behind the "real" Outcome where the real development occurs. And finally,
then Outcome would gain a Boost.Build veneer...
libraries are much easier to review if they are already in their final Boost form, ready to be copied into $BOOST_ROOT/libs/$library. It's understandable that people don't want to invest into that final step until the library is accepted... but it should also be understandable that going that extra mile is a sign of commitment that speaks positively about the author and can therefore tilt reviewers toward acceptance.
Boost-lite provides the cmake based build, config, test, Boost.Test alternative, install, partial preprocess single header include generation and much of the internal implemention classes used by Outcome. However if accepted then Outcome would gain a Boost.Build veneer and be auto generated by script from the standalone Outcome same as Boost.ASIO is, and so I figure none of that is in scope for this review.
This doesn't exactly inspire confidence.
When a library enters Boost and is distributed in Boost releases, there's no longer any point of it duplicating Boost functionality under the hood (without a very good reason); while not requiring Boost is understandable for standalone use, in a Boost release Boost is by definition already available.
Almost all usage of boost-lite is substitutable by Boost. You can switch it with a macro. That code path is stale and likely won't work right now, but it wouldn't take much effort to restore it if the peer review demanded it. You may remember some years ago I developed a set of switchable STL implementation bindings to let one switch a library between Boost or the C++ 11 STL. Outcome uses those.
then Outcome would [...] be auto generated by script from the standalone Outcome same as Boost.ASIO is
carries the unfortunate implication that Outcome in Boost may be, similarly to Boost.ASIO, a second-class citizen that may lag behind the "real" Outcome where the real development occurs.
This is a chimera. Standalone ASIO has only diverged significantly from Boost.ASIO because standalone ASIO has become unstable due to all the changes flowing down from developing the Networking TS. Chris has, quite rightly, kept Boost.ASIO insulated from all that profound change. If the Networking TS were not happening, standalone ASIO and Boost.ASIO would be in sync, and for the record, if you really want an up to date Boost.ASIO, you can go run ASIO's Boost.ASIO generation scripts and you'll get one. Furthermore, Outcome is itself already generated by script. When you include Outcome, you are including the pregenerated edition. Therefore generating a "boost flavoured" edition of same is no different. That means both standalone Outcome and any potential Boost.Outcome are exactly the same class of citizen, which is to say first class.
And finally,
then Outcome would gain a Boost.Build veneer...
libraries are much easier to review if they are already in their final Boost form, ready to be copied into $BOOST_ROOT/libs/$library. It's understandable that people don't want to invest into that final step until the library is accepted... but it should also be understandable that going that extra mile is a sign of commitment that speaks positively about the author and can therefore tilt reviewers toward acceptance.
Once again you jump to conclusions. Outcome is header only and follows the Boost directory layout. You can drop it into a Boost distro if you really want to, but there is no point, it makes no use of Boost. All the conformance unit tests sit in a single file, and because I knew there would be the above complaint about missing Jamfiles, I made a shell and batch script that lets you compile the unit tests and run them without needing cmake. You'll find one for POSIX and one for Windows in the test directory. Quite a few libraries submitted to Boost recently came with cmake instead of Boost.Build. Only after approval did the author make the Jamfiles. There is precedent here. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Niall Douglas wrote:
Furthermore, Outcome is itself already generated by script. When you include Outcome, you are including the pregenerated edition. Therefore generating a "boost flavoured" edition of same is no different.
That means both standalone Outcome and any potential Boost.Outcome are exactly the same class of citizen, which is to say first class
Well, not quite. Let's suppose it's accepted. There is a repo under boostorg, boostorg/outcome, that hopefully contains headers. Someone issues a pull request against one of those headers. To incorporate this PR, you need to backport it into the primary Outcome source, then rerun the boostification script, then commit the result to the boostorg repo, then check whether the changes match the PR. Correct? Unless you also have a de-boostification script that would somehow allow you to merge the PR and then automatically port the changes to the primary repo.
Furthermore, Outcome is itself already generated by script. When you include Outcome, you are including the pregenerated edition. Therefore generating a "boost flavoured" edition of same is no different.
That means both standalone Outcome and any potential Boost.Outcome are exactly the same class of citizen, which is to say first class
Well, not quite. Let's suppose it's accepted. There is a repo under boostorg, boostorg/outcome, that hopefully contains headers. Someone issues a pull request against one of those headers. To incorporate this PR, you need to backport it into the primary Outcome source, then rerun the boostification script, then commit the result to the boostorg repo, then check whether the changes match the PR.
Correct?
It depends on what conditions the peer review places on Outcome to be accepted. If the repo were accepted as is presented - and my hope would be that it is - then users will be using it directly. However even if it isn't, the situation with ASIO hasn't been particularly problematic. Chris figures out an equivalent patch and applies it to standalone ASIO. It's ok. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Niall Douglas wrote:
Well, not quite. Let's suppose it's accepted. There is a repo under boostorg, boostorg/outcome, that hopefully contains headers. Someone issues a pull request against one of those headers. To incorporate this PR, you need to backport it into the primary Outcome source, then rerun the boostification script, then commit the result to the boostorg repo, then check whether the changes match the PR.
Correct?
It depends on what conditions the peer review places on Outcome to be accepted.
If the repo were accepted as is presented - and my hope would be that it is - then users will be using it directly.
This answer makes no sense to me. Boost acceptance implies that the library is distributed as part of a Boost release. How could users be using it directly?
On 05/14/2017 01:44 PM, Niall Douglas via Boost wrote:
Boost-lite provides the cmake based build, config, test, Boost.Test alternative, install, partial preprocess single header include generation and much of the internal implemention classes used by Outcome. However if accepted then Outcome would gain a Boost.Build veneer and be auto generated by script from the standalone Outcome same as Boost.ASIO is, and so I figure none of that is in scope for this review.
It is still unclear exactly what code is under review. What does this potentially auto-generated code that might enter Boost look like? Is the code in the "attic" folder under review?
It is still unclear exactly what code is under review. What does this potentially auto-generated code that might enter Boost look like?
The auto generated file is include/boost/outcome/outcome_v1.0.hpp. It is a single file representing the entire of Outcome, including all dependencies. It is automatically used by include/boost/outcome/outcome.hpp if the conditions are right.
Is the code in the "attic" folder under review?
There is more than one attic folder incidentally. Stuff in attic can be safely disregarded for this review. Outcome originally documented how to extend basic_monad with custom semantics by end users to make new monads, but Hana does all that stuff much better, so extensibility is no longer advertised. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
participants (5)
-
Bjorn Reese -
Glen Fernandes -
Niall Douglas -
Peter Dimov -
Robert Ramey