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 any_image_view::const_t #526

Merged
merged 4 commits into from
Mar 18, 2021

Conversation

sdebionne
Copy link
Contributor

@sdebionne sdebionne commented Oct 21, 2020

Description

Fix any_image_view<>::const_t that currently returns an invalid type, see the provided test case that fails on develop.

Tasklist

  • Add test case(s)
  • Ensure all CI builds pass
  • Review and approve

Copy link
Member

@mloskot mloskot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An interesting catch! LGTM, just a few nitpicks

test/extension/dynamic_image/any_image_view.cpp Outdated Show resolved Hide resolved
@sdebionne
Copy link
Contributor Author

I found an other issue similar to #486 (aka any_image_view<...> v1 = v0 fails to compile) and added the tests accordingly.

@sdebionne
Copy link
Contributor Author

I thought using inheriting constructors was the right think to do, but it looks like it fails with GCC5, is it C++11 feature complete?

@mloskot
Copy link
Member

mloskot commented Oct 23, 2020

https://gcc.gnu.org/projects/cxx-status.html#cxx11 says

GCC 4.8.1 was the first feature-complete implementation of the 2011 C++ standard

However, we have dropped support from GCC 4.8 as its C++11 support is unusable for GIL (and Boost.MP11 too), see #296

Perhaps GCC 5 is lacking or suffering a bug.

@sdebionne
Copy link
Contributor Author

Do you want to support GCC5, in which case I can revert to declaring every constructors (including the missing one to support initializing construction)?

@mloskot
Copy link
Member

mloskot commented Oct 23, 2020

Do you want to support GCC5

Good question.

Dropping support for GCC 5 could be part of switching to C++17 which was seeded on the Slack, https://cpplang.slack.com/archives/CSVT0STV2/p1588870474195500
I'd not object it, but we need to collect opinions from others (may be worth posting to Boost or Boost GIL mailnig list)
/cc @chhenning @lpranam @stefanseefeld

@sdebionne
Copy link
Contributor Author

Not sure if this is the one, but there are bugs related to inheriting constructors in GCC 5:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62310

@mloskot
Copy link
Member

mloskot commented Oct 26, 2020

@sdebionne Any workaround to keep compatible w/ GCC 5 is best option for the time being.

UPDATE: See also @pdimov 's suggestion here https://lists.boost.org/Archives/boost/2020/10/250222.php

@sdebionne
Copy link
Contributor Author

I am OK with both approaches, aka workaround or dropping GCC 5, but the later and making C++ 14 the new language std requirement is more appealing to me.

@mloskot
Copy link
Member

mloskot commented Oct 29, 2020

@sdebionne Yes, let's prepare for the step towards C++14, https://lists.boost.org/boost-gil/2020/10/0469.php

@sdebionne
Copy link
Contributor Author

@mloskot To clarify for 1.75, are we dropping GCC 5 (in which case the checks should pass and this PR could be merged) or do you prefer a workaround? Or are we too late anyway?

@pdimov
Copy link
Member

pdimov commented Nov 6, 2020

1.75 has just been frozen for beta.

@mloskot
Copy link
Member

mloskot commented Nov 8, 2020

@sdebionne

To clarify for 1.75, are we dropping GCC 5

No, I don't think it's right to drop GCC 5 or start requiring C++14 from 1.75.
My plan is to add notice to Boost 1.75 release notes that GCC 5 support will be dropped in next release and C++14 will likely be required version too.

mloskot added a commit that referenced this pull request Nov 10, 2020
@mloskot
Copy link
Member

mloskot commented Nov 10, 2020

I've announced our plans in the release notes, 8b1c2d3 and boostorg/website#562

mloskot added a commit that referenced this pull request Feb 19, 2021
@sdebionne
Copy link
Contributor Author

If I am correct, I need to close and reopen the PR to get the updated pipeline without GCC5? Or is there a better way?

@mloskot
Copy link
Member

mloskot commented Mar 2, 2021

@sdebionne I think you need to rebase your sdebionne:fix-any-image-view-const-t branch and git push --force it to update this PR, then it will rebuild.

@sdebionne
Copy link
Contributor Author

Done. But I still see gcc-5 on Azure and gcc-4.9 on Github Actions, both versions probably impacted by the bug mentioned before.

mloskot added a commit to mloskot/gil that referenced this pull request Mar 2, 2021
mloskot added a commit to mloskot/gil that referenced this pull request Mar 2, 2021
@mloskot
Copy link
Member

mloskot commented Mar 2, 2021

@sdebionne Sorry, there had been no update to our CI. Fixing in #572

mloskot added a commit that referenced this pull request Mar 2, 2021
@mloskot
Copy link
Member

mloskot commented Mar 2, 2021

@sdebionne The #572 has been merged. If you could update this PR with the develop, then it should build fine.

@sdebionne
Copy link
Contributor Author

How should I interpret the result of the CI:

  • Azure (ubuntu1604_gcc6_cxx14_cmake) fails
  • GA (gcc-6, 11,14,1z, ubuntu-16.04, g++-6) passes

meshtag pushed a commit to meshtag/gil that referenced this pull request Mar 17, 2021
@sdebionne
Copy link
Contributor Author

LGTM. Shall we merge?

If you are asking me, I would say yes! Or do I need to rebase?

@mloskot mloskot added cat/bug But reports and bug fixes ext/dynamic_image boost/gil/extension/dynamic_image labels Mar 18, 2021
@mloskot mloskot added this to the Boost 1.77+ milestone Mar 18, 2021
@mloskot mloskot merged commit b8564e2 into boostorg:develop Mar 18, 2021
@mloskot
Copy link
Member

mloskot commented Mar 18, 2021

Thanks

meshtag pushed a commit to meshtag/gil that referenced this pull request Mar 23, 2021
meshtag pushed a commit to meshtag/gil that referenced this pull request Mar 23, 2021
meshtag pushed a commit to meshtag/gil that referenced this pull request Mar 23, 2021
Use inherited constructors in any_image as well
meshtag pushed a commit to meshtag/gil that referenced this pull request Apr 21, 2021
meshtag pushed a commit to meshtag/gil that referenced this pull request Apr 21, 2021
meshtag pushed a commit to meshtag/gil that referenced this pull request Apr 21, 2021
Use inherited constructors in any_image as well
meshtag pushed a commit to meshtag/gil that referenced this pull request Apr 21, 2021
meshtag pushed a commit to meshtag/gil that referenced this pull request Apr 21, 2021
meshtag pushed a commit to meshtag/gil that referenced this pull request Apr 21, 2021
Use inherited constructors in any_image as well
meshtag pushed a commit to meshtag/gil that referenced this pull request Apr 22, 2021
meshtag pushed a commit to meshtag/gil that referenced this pull request Apr 22, 2021
meshtag pushed a commit to meshtag/gil that referenced this pull request Apr 22, 2021
Use inherited constructors in any_image as well
sdebionne pushed a commit to sdebionne/gil-reformated that referenced this pull request May 26, 2021
sdebionne pushed a commit to sdebionne/gil-reformated that referenced this pull request May 26, 2021
sdebionne pushed a commit to sdebionne/gil-reformated that referenced this pull request Jun 23, 2021
sdebionne pushed a commit to sdebionne/gil-reformated that referenced this pull request Jun 23, 2021
@sdebionne sdebionne deleted the fix-any-image-view-const-t branch May 4, 2022 06:46
striezel pushed a commit to striezel-stash/gil that referenced this pull request May 6, 2022
mloskot added a commit that referenced this pull request May 7, 2022
…#664)

See discussion in #526

Cherry-pick of 8b1c2d3 from develop.

Co-authored-by: Mateusz Łoskot <mateusz@loskot.net>
@mloskot mloskot mentioned this pull request May 12, 2022
6 tasks
@mloskot
Copy link
Member

mloskot commented May 20, 2022

@sdebionne

I am OK with both approaches, aka workaround or dropping GCC 5, but the later and making C++ 14 the new language std requirement is more appealing to me.

Let's do it for Boost 1.80. I have moved the planning towards C++14/17 to discussion here #676

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/dynamic_image boost/gil/extension/dynamic_image
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants