[modularization] Recommendations
Hello, My last set of recommendations is here: http://thread.gmane.org/gmane.comp.lib.boost.devel/245078/focus=245806 The current graph of strongly connected modules is: http://www.steveire.com/boost/2014_june8_all.dot http://www.steveire.com/boost/2014_june8_all.png The type_traits mpl typeof core form a closed cylce. I consider this a small problem and I recommend deferring efforts to resolve it for now and focusing on the other edges which impact the rest of the graph. It may not be realistic to resolve every cycle or doing so may involve merging modules etc. I recommend deferring that discussion/work until after the major cycle-forming-edges are resolved. Group the above in the graph as a single node for now, removing many cycles: http://www.steveire.com/boost/2014_june8_with_kernel.png http://www.steveire.com/boost/2014_june8_with_kernel.dot Recommendation 1) Move parenthesized_type.hpp from parameter to core This removes the dependency from concept_check on parameter. Recommendation 2) Move implicit_cast.hpp and cast.hpp from conversion to core This also requires moving select_type.hpp from detail to core. This allows removal of the dependencies from iterator and range to conversion. As a side effect, it leaves lexical_cast as the only remaining public header in the conversion library. You can decide what impact that has for the recent 'Extracting lexical_cast from conversion' discussion. This requires removing the include of numeric/conversion/cast.hpp from cast.hpp. The content of that was moved to the numeric library in 2005 and the include was left behind for source compatibility. The result of the above recommendations is: http://www.steveire.com/boost/2014_june8_conversion_rdep.png http://www.steveire.com/boost/2014_june8_conversion_rdep.dot Recommendation 3) Remove use of algorithm from range Range includes boost/algorithm/string/config.hpp for no known purpose. According to git log -S BOOST_STRING_TYPENAME that define was never used in the range library. According to the history of boost/algorithm/string/config.hpp it never really had anything else useful. It might have made sense to include it for the Borland portability with the BOOST_STRING_TYPENAME, but that define was never used in range. Simply remove the use of the boost/algorithm/string/config.hpp header. Use boost/type_traits/detail/yes_no_type.hpp instead of boost/algorithm/string/yes_no_type.hpp in include/boost/range/detail/collection_traits_detail.hpp The resulting strongly connected graph is http://www.steveire.com/boost/2014_june8_range_deps.png http://www.steveire.com/boost/2014_june8_range_deps.dot There are more recommendations which can be made to reduce the strongly connected graph further, but I think the above are enough for now, so let's see if we can get through those first. Thanks, Steve.
Stephen Kelly wrote:
Range includes
boost/algorithm/string/config.hpp
for no known purpose. According to
git log -S BOOST_STRING_TYPENAME
that define was never used in the range library.
I checked again and I was wrong about this. The define is used in Boost.Range, but now, given updated compiler requirements, it can be replaced with BOOST_DEDUCED_TYPENAME. I recommend doing that. Thanks, Steve.
Thanks, Stephen, for the updated recommendations. Stephen Kelly wrote:
Recommendation 2) Move implicit_cast.hpp and cast.hpp from conversion to core
This also requires moving select_type.hpp from detail to core.
Actually, select_type.hpp is not being used in cast.hpp at all, as far as I can see. The include can just be removed. cast.hpp probably also needs to die, the polymorphic casts going into their own header. This will be post-release because it will break code, though. Inclusion report for <boost/cast.hpp> (in module conversion): from gil: <boost/gil/extension/toolbox/color_spaces/hsv.hpp> <boost/gil/extension/toolbox/color_spaces/lab.hpp> <boost/gil/extension/toolbox/color_spaces/xyz.hpp> <boost/gil/extension/toolbox/color_spaces/ycbcr.hpp> from range: <boost/range/adaptor/type_erased.hpp> <boost/range/any_range.hpp> <boost/range/detail/any_iterator.hpp> <boost/range/detail/any_iterator_wrapper.hpp> from statechart: <boost/statechart/custom_reaction.hpp> <boost/statechart/detail/reaction_dispatcher.hpp> <boost/statechart/event.hpp> <boost/statechart/simple_state.hpp> <boost/statechart/state_machine.hpp>
Peter Dimov wrote:
cast.hpp probably also needs to die, the polymorphic casts going into their own header. This will be post-release because it will break code, though.
Actually it doesn't need to be a breaking change. You can create a new polymorphic_cast.hpp in core and #include that in cast.hpp, and move that cast.hpp (which now only contains two includes) to numeric. Then you can deprecate cast.hpp with a pragma warning and port any uses of it to include polymorphic_cast.hpp instead. Non breaking change. Thanks, Steve.
Stephen Kelly wrote:
Peter Dimov wrote:
cast.hpp probably also needs to die, the polymorphic casts going into their own header. This will be post-release because it will break code, though.
Actually it doesn't need to be a breaking change.
You can create a new polymorphic_cast.hpp in core and #include that in cast.hpp, and move that cast.hpp (which now only contains two includes) to numeric. Then you can deprecate cast.hpp with a pragma warning and port any uses of it to include polymorphic_cast.hpp instead. Non breaking change.
Yes, you are right. Moving cast.hpp to numeric/conversion is a better way to deal with it. There'll be no need to remove the numeric/ include then, and it will truly be a non-breaking change. Re moving *_cast.hpp to Core, I actually prefer the other way, moving lexical_cast to its own module. Conversion will then drop to level 2, same as Core.
2014-06-08 18:15 GMT+04:00 Peter Dimov <lists@pdimov.com>:
Stephen Kelly wrote:
Peter Dimov wrote:
cast.hpp probably also needs to die, the polymorphic casts going into > their own header. This will be post-release because it will break code, > though.
Actually it doesn't need to be a breaking change.
You can create a new polymorphic_cast.hpp in core and #include that in cast.hpp, and move that cast.hpp (which now only contains two includes) to numeric. Then you can deprecate cast.hpp with a pragma warning and port any uses of it to include polymorphic_cast.hpp instead. Non breaking change.
Yes, you are right. Moving cast.hpp to numeric/conversion is a better way to deal with it. There'll be no need to remove the numeric/ include then, and it will truly be a non-breaking change.
This does not look right. polymorpic_cast and polymotphic_downcast do not look like they belong to Numeric. They do not deal with numbers at all.
Re moving *_cast.hpp to Core, I actually prefer the other way, moving lexical_cast to its own module. Conversion will then drop to level 2, same as Core.
Most part of the Conversion dependencies are related to the LexicalCast. Moving it into a separate module looks reasonable. It is already represented as a separate library on the website. -- Best regards, Antony Polukhin
Antony Polukhin wrote:
This does not look right. polymorpic_cast and polymotphic_downcast do not look like they belong to Numeric. They do not deal with numbers at all.
The proposal is: - move the polymorphic casts into their own header, in conversion, boost/polymorphic_cast.hpp; - move boost/cast.hpp, which is now #include <boost/implicit_cast.hpp> #include <boost/polymorphic_cast.hpp> #include <boost/numeric/conversion/cast.hpp> into numeric/conversion, pending deprecation.
Most part of the Conversion dependencies are related to the LexicalCast. Moving it into a separate module looks reasonable. It is already represented as a separate library on the website.
Everyone seems to agree on that, so what do you need to move forward with it? Will just an empty 'lexical_cast' repository be enough, or do you need me to do something else? Perhaps cloning the current contents of 'conversion' into 'lexical_cast' may be a better starting point?
2014-06-08 18:51 GMT+04:00 Peter Dimov <lists@pdimov.com>:
Antony Polukhin wrote:
This does not look right. polymorpic_cast and polymotphic_downcast do not
look like they belong to Numeric. They do not deal with numbers at all.
The proposal is:
- move the polymorphic casts into their own header, in conversion, boost/polymorphic_cast.hpp; - move boost/cast.hpp, which is now
#include <boost/implicit_cast.hpp> #include <boost/polymorphic_cast.hpp> #include <boost/numeric/conversion/cast.hpp>
into numeric/conversion, pending deprecation.
This looks reasonable. BTW what about the new Conversion library (was it accepted?)? If it was accepted, then it will add a lot of dependencies and it would be better to separate it from the old conversion library.
Most part of the Conversion dependencies are related to the LexicalCast.
Moving it into a separate module looks reasonable. It is already represented as a separate library on the website.
Everyone seems to agree on that, so what do you need to move forward with it? Will just an empty 'lexical_cast' repository be enough, or do you need me to do something else? Perhaps cloning the current contents of 'conversion' into 'lexical_cast' may be a better starting point?
Repo with cloned contest of conversion would be great. I'll do the remaining part of the work tomorrow (remove the files of other libraries, update the docs... ). Please make a repo. -- Best regards, Antony Polukhin
Antony Polukhin wrote:
Repo with cloned contest of conversion would be great. I'll do the remaining part of the work tomorrow (remove the files of other libraries, update the docs... ).
Please make a repo.
Done. You have a repo lexical_cast that should be a carbon copy of conversion, and a submodule lexical_cast in develop. There's no submodule in master yet, let me know when you think I should create it there as well.
On 6/8/2014 1:58 PM, Antony Polukhin wrote:
2014-06-08 18:51 GMT+04:00 Peter Dimov <lists@pdimov.com>:
Antony Polukhin wrote:
This does not look right. polymorpic_cast and polymotphic_downcast do not
look like they belong to Numeric. They do not deal with numbers at all.
The proposal is:
- move the polymorphic casts into their own header, in conversion, boost/polymorphic_cast.hpp; - move boost/cast.hpp, which is now
#include <boost/implicit_cast.hpp> #include <boost/polymorphic_cast.hpp> #include <boost/numeric/conversion/cast.hpp>
into numeric/conversion, pending deprecation.
This looks reasonable.
BTW what about the new Conversion library (was it accepted?)? If it was accepted, then it will add a lot of dependencies and it would be better to separate it from the old conversion library.
The new library, called Convert ( not Conversion ), was accepted.
On 6/8/2014 5:59 PM, Peter Dimov wrote:
Edward Diener wrote:
The new library, called Convert ( not Conversion ), was accepted.
That's probably going to create a lot of confusion. :-)
Perhaps but Conversion is really 'cast' and 'lexical_cast' so maybe it is badly named and should be called the 'Cast' or 'Casting' library instead purely based on its internal classes.
2014-06-08 15:44 GMT+04:00 Peter Dimov <lists@pdimov.com>:
Thanks, Stephen, for the updated recommendations.
Stephen Kelly wrote:
Recommendation 2) Move implicit_cast.hpp and cast.hpp from conversion to
core
This also requires moving select_type.hpp from detail to core.
Actually, select_type.hpp is not being used in cast.hpp at all, as far as I can see. The include can just be removed.
cast.hpp probably also needs to die, the polymorphic casts going into their own header. This will be post-release because it will break code, though.
Inclusion report for <boost/cast.hpp> (in module conversion):
from gil: <boost/gil/extension/toolbox/color_spaces/hsv.hpp> <boost/gil/extension/toolbox/color_spaces/lab.hpp> <boost/gil/extension/toolbox/color_spaces/xyz.hpp> <boost/gil/extension/toolbox/color_spaces/ycbcr.hpp>
from range: <boost/range/adaptor/type_erased.hpp> <boost/range/any_range.hpp> <boost/range/detail/any_iterator.hpp> <boost/range/detail/any_iterator_wrapper.hpp>
from statechart: <boost/statechart/custom_reaction.hpp> <boost/statechart/detail/reaction_dispatcher.hpp> <boost/statechart/event.hpp> <boost/statechart/simple_state.hpp> <boost/statechart/state_machine.hpp>
Made pull requests for those libraries to use polymorphic_cast.hpp instead of cast.hpp: https://github.com/boostorg/gil/pull/9 https://github.com/boostorg/statechart/pull/1 https://github.com/boostorg/range/pull/10 Moved cast.hpp to the NumericCast library: https://github.com/boostorg/numeric_conversion/pull/2 -- Best regards, Antony Polukhin
Antony Polukhin wrote:
Made pull requests for those libraries to use polymorphic_cast.hpp instead of cast.hpp: https://github.com/boostorg/gil/pull/9 https://github.com/boostorg/statechart/pull/1 https://github.com/boostorg/range/pull/10
Moved cast.hpp to the NumericCast library: https://github.com/boostorg/numeric_conversion/pull/2
I can merge these if there are no objections.
2014-06-09 15:21 GMT+04:00 Peter Dimov <lists@pdimov.com>:
Antony Polukhin wrote:
Made pull requests for those libraries to use polymorphic_cast.hpp instead of cast.hpp: https://github.com/boostorg/gil/pull/9 https://github.com/boostorg/statechart/pull/1 https://github.com/boostorg/range/pull/10
Moved cast.hpp to the NumericCast library: https://github.com/boostorg/numeric_conversion/pull/2
I can merge these if there are no objections.
I have no objections, but please add the new LexicalCast module to the master branch of boostorg/boost project first. -- Best regards, Antony Polukhin
Le 08/06/14 12:21, Stephen Kelly a écrit :
Recommendation 3) Remove use of algorithm from range
Range includes
boost/algorithm/string/config.hpp
for no known purpose. According to
git log -S BOOST_STRING_TYPENAME
that define was never used in the range library. According to the history of
boost/algorithm/string/config.hpp
it never really had anything else useful. It might have made sense to include it for the Borland portability with the BOOST_STRING_TYPENAME, but that define was never used in range.
Simply remove the use of the
boost/algorithm/string/config.hpp
header.
Use
boost/type_traits/detail/yes_no_type.hpp
instead of
boost/algorithm/string/yes_no_type.hpp
in
include/boost/range/detail/collection_traits_detail.hpp
Hi, I agree with this modifications. Could we decide if this is the way to resolve this dependency? Best, Vicente
Vicente J. Botet Escriba wrote:
Le 08/06/14 12:21, Stephen Kelly a écrit :
Recommendation 3) Remove use of algorithm from range
Range includes
boost/algorithm/string/config.hpp
... Hi,
I agree with this modifications.
Could we decide if this is the way to resolve this dependency?
Our general procedure should be that the decision if or when to resolve a dependency is up to the library maintainers.
On Mon, Jun 9, 2014 at 11:49 AM, Peter Dimov <lists@pdimov.com> wrote:
Vicente J. Botet Escriba wrote:
Le 08/06/14 12:21, Stephen Kelly a écrit :
Recommendation 3) Remove use of algorithm from range
Range includes
boost/algorithm/string/config.hpp
...
Hi,
I agree with this modifications.
Could we decide if this is the way to resolve this dependency?
Our general procedure should be that the decision if or when to resolve a dependency is up to the library maintainers.
I am a Boost.Range maintainer and I don't mind having the dependency moved or removed. My delayed response is due to considering the way in which I might assist best. I would like to improve the component breakdown and hence modularisation effort. In addition to the dependency on algorithm, I think Boost.Range would be improved by moving the algorithms into Boost.Algorithm, and the tokenized adaptor should be moved into Boost.Regex. Currently some C++11 algorithms are missing from Boost.Range but present in Boost.Algorithm whereas C++03 algorithms all live inside Boost.Range. I'd like to see Boost.Range become smaller and encourage other components to provide range algorithms and adaptors. This is why the richness of the Boost.Range adaptors is a little disappointing, I am not keen on providing rich adaptors that couple other components. The adaptor idiom should be adopted elsewhere. I don't want to do less to help Boost and the lack of write access to other components makes this reorganisation difficult for me to accomplish. Obviously I would wish to discuss and gain support from the appropriate library maintainers, but the write access issue is a barrier. I believe that the strong rights to access to the component I am responsible for combined with low rights to other components drives me toward making design decisions that are suboptimal. I wonder if this has the same effect with other developers. If so should we reconsider re-enabling broader write access and encourage a less formal peer collaboration? I would like to join the community maintenance team and have broader write access? I'd be delighted to help modularise Boost.Range and help out where I can with other maintenance issues. Over the last few months I've been able to dedicate significantly more time to Boost. In summary, I'm happy to move aside to so that those working on the modularisation effort can make progress. I am equally happy to directly help. Regards, Neil Groves
Le 09/06/14 13:25, Neil Groves a écrit :
On Mon, Jun 9, 2014 at 11:49 AM, Peter Dimov <lists@pdimov.com> wrote:
Vicente J. Botet Escriba wrote:
Le 08/06/14 12:21, Stephen Kelly a écrit :
Recommendation 3) Remove use of algorithm from range
Range includes
boost/algorithm/string/config.hpp
...
Hi,
I agree with this modifications.
Could we decide if this is the way to resolve this dependency?
Our general procedure should be that the decision if or when to resolve a dependency is up to the library maintainers.
I am a Boost.Range maintainer and I don't mind having the dependency moved or removed. My delayed response is due to considering the way in which I might assist best. I would like to improve the component breakdown and hence modularisation effort. In addition to the dependency on algorithm, I think Boost.Range would be improved by moving the algorithms into Boost.Algorithm, and the tokenized adaptor should be moved into Boost.Regex. Currently some C++11 algorithms are missing from Boost.Range but present in Boost.Algorithm whereas C++03 algorithms all live inside Boost.Range. I'd like to see Boost.Range become smaller and encourage other components to provide range algorithms and adaptors. This is why the richness of the Boost.Range adaptors is a little disappointing, I am not keen on providing rich adaptors that couple other components. The adaptor idiom should be adopted elsewhere. I don't want to do less to help Boost and the lack of write access to other components makes this reorganisation difficult for me to accomplish. Obviously I would wish to discuss and gain support from the appropriate library maintainers, but the write access issue is a barrier. Neil I think that the most urgent is in removing the dependencies that make cycles. Once this is done, xe could refactor more if this makes Boost more manageable. I believe that the strong rights to access to the component I am responsible for combined with low rights to other components drives me toward making design decisions that are suboptimal. I wonder if this has the same effect with other developers. If so should we reconsider re-enabling broader write access and encourage a less formal peer collaboration? You could always fork and do pull requests, even if this is not optimal. I would like to join the community maintenance team and have broader write access? I'd be delighted to help modularise Boost.Range and help out where I can with other maintenance issues. Over the last few months I've been able to dedicate significantly more time to Boost.
In summary, I'm happy to move aside to so that those working on the modularisation effort can make progress. I am equally happy to directly help.
I think it is enough you request to be a member of the community maintenece team explicitly. Thanks, Vicente
participants (6)
-
Antony Polukhin -
Edward Diener -
Neil Groves -
Peter Dimov -
Stephen Kelly -
Vicente J. Botet Escriba