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

Fix conflicting definitions from io/dynamic_io_new.hpp and toolbox/dynamic_images.hpp #185

Merged

Conversation

mloskot
Copy link
Member

@mloskot mloskot commented Dec 11, 2018

The problem appears to be due to the same definitions copied from one part of the library to the other.
The definitions have been shuffled to fix the compilation, but purely based on searching the code for what is used where, thus without confidence where those belong by author's intention.

Fixes #183

Tasklist


  1. extension/toolbox/dynamic_images.hpp - removed construct_matched which is defined in io/dynamic_io_new.hpp and it is not used anywhere in extension/toolbox/*.hpp , but only in extension/io.

image

  1. io/dynamic_io_new.hpp - removed any_image_*_t structures which are defined and used in extension/toolbox/dynamic_images.hpp, but nowhere else.

image

@mloskot mloskot added the cat/bug But reports and bug fixes label Dec 11, 2018
@mloskot mloskot changed the title Fix conflicting definitions from io/dynamic_io_new.hpp and toolbox/dynamic_images.hpp WIP: Fix conflicting definitions from io/dynamic_io_new.hpp and toolbox/dynamic_images.hpp Dec 11, 2018
…namic_images.hpp

The problem appears to be due to the same definitions copied from one
part of the library to the other.
The definitions have been shuffled to fix the compilation,
but purely based on searching the code for what is used where,
thus without confidence where those belong by author's intention.

Fixes boostorg#183
@mloskot mloskot force-pushed the ml/fix-183-conflicting-definitions branch from 1b37b62 to ece7150 Compare December 11, 2018 20:56
@mloskot mloskot changed the title WIP: Fix conflicting definitions from io/dynamic_io_new.hpp and toolbox/dynamic_images.hpp Fix conflicting definitions from io/dynamic_io_new.hpp and toolbox/dynamic_images.hpp Dec 11, 2018
@mloskot
Copy link
Member Author

mloskot commented Dec 11, 2018

@stefanseefeld this one is now ready to review.

CI buils are still ongoing for the boostorg, but my personal one has finished green https://travis-ci.org/mloskot/gil/builds/466699298

@mloskot mloskot added this to the Boost 1.70+ milestone Dec 12, 2018
@mloskot mloskot added ext/io boost/gil/extension/io/ ext/toolbox boost/gil/extension/toolbox/ labels Dec 12, 2018
@sdebionne
Copy link
Contributor

Since the old version of io/dynamic_io_new.hpp has been removed, should io/dynamic_io_new.hpp be io/dynamic_io.hpp really?
I fall into this trap to, and took the opposite direction, that is removing dynamic_io_new.hpp and including the toolbox/dynamic_images.hpp where needed.

@mloskot
Copy link
Member Author

mloskot commented Dec 12, 2018

Since the old version of io/dynamic_io_new.hpp has been removed, should io/dynamic_io_new.hpp be io/dynamic_io.hpp really?

Possibly, or even io/dynamic.hpp.

removing dynamic_io_new.hpp and including the toolbox/dynamic_images.hpp where needed.

That seems like an arguable direction. The io/dynamic_io_new.hpp is a core header. The toolbox is an extension. Since extensions are optional, even those officially released, and nothing in the core should depend on extensions (there is one or two places where this policy has been broken though, I think, to be fixed).

@mloskot
Copy link
Member Author

mloskot commented Dec 12, 2018

@stefanseefeld Thanks for the review.

@sdebionne Do you agree see any problems; have any objections against merging this PR? e.g. if you think it would seriously break things on your end of GIL usage.

@sdebionne
Copy link
Contributor

Nope, nothing serious, go ahead! Thanks for asking!

@mloskot
Copy link
Member Author

mloskot commented Dec 12, 2018

Great, merging.

...and praying

@mloskot mloskot merged commit 2250b71 into boostorg:develop Dec 12, 2018
@mloskot mloskot deleted the ml/fix-183-conflicting-definitions branch December 12, 2018 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat/bug But reports and bug fixes ext/io boost/gil/extension/io/ ext/toolbox boost/gil/extension/toolbox/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conflicting definitions in io/dynamic_io_new.hpp and extension/toolbox/dynamic_images.hpp
3 participants