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.

Sign up for GitHub

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

Jump to bottom

5 ➡️ 4 - EnumIface: suppress deprecation warning (#540) #600

Closed
wants to merge 8 commits into from
Closed

5 ➡️ 4 - EnumIface: suppress deprecation warning (#540) #600

wants to merge 8 commits into from

Conversation

Crola1702
Copy link
Contributor

🦟 Bug fix

Fixes #505

Summary

Backport of #540

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@scpeters @Crola1702
EnumIface: suppress deprecation warning (#540)
1e1c34b 
The EnumIterator inherits from std::iterator, which has
been deprecated in c++17. There is a fix in gz-common6
but it breaks API, so it can't be merged to Garden. So
just suppress the deprecation warning in Garden.

Signed-off-by: Steve Peters <[email protected]>
@Crola1702 Crola1702 requested a review from marcoag as a code owner April 25, 2024 14:44
@Crola1702 Crola1702 requested a review from scpeters April 25, 2024 14:45
@github-actions github-actions bot added 🌱 garden Ignition Garden 🎵 harmonic Gazebo Harmonic labels Apr 25, 2024
@Crola1702 Crola1702 changed the base branch from gz-common5 to ign-common4 April 25, 2024 14:45
azeey
azeey requested changes Apr 25, 2024
View reviewed changes
include/gz/common/EnumIface.hh Outdated
@@ -142,6 +142,7 @@ namespace ignition
/// std::cout << "Type++ Name[" << myTypeIface.Str(*i) << "]\n";
/// }
/// \verbatim
GZ_UTILS_WARN_IGNORE__DEPRECATED_DECLARATION
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll need to include https://github.com/gazebosim/gz-utils/blob/ign-utils1/include/gz/utils/SuppressWarning.hh and use the IGN prefix. Unfortunately, these macros don't have a GZ prefixed alias in ign-utils1.

@Crola1702 Crola1702 force-pushed the Crola1702/5_to_4_25-04-2024 branch from 4be1c1e to c95da7c Compare April 26, 2024 13:41
@Crola1702
Change header to ignition
25fda70 
Signed-off-by: Crola1702 <[email protected]>
@scpeters
Copy link
Member

scpeters commented Apr 29, 2024 •
edited
Loading

This change also requires calling target_link_library on the utils library, which I've done in 6652132 in a way that will minimize the diff when merging forward to gz-common5. Feel free to cherry-pick into this branch.

I think it is safe for our Ubuntu packages because the libignition-common4-core-dev already depends on libignition-utils1-dev (see common4 control file)

scpeters and others added 2 commits May 1, 2024 18:56
@scpeters @Crola1702
Link core library publicly to gz-utils
c336c7b 
Needed to use warning suppression in public header file.

Signed-off-by: Steve Peters <[email protected]>
@Crola1702
Change include header to gz
a2cfb85 
Signed-off-by: Crola1702 <[email protected]>
azeey
azeey requested changes May 7, 2024
View reviewed changes
src/CMakeLists.txt Outdated Show resolved Hide resolved
@azeey
Copy link
Contributor

azeey commented May 7, 2024

A version that doesn't use the ign-utils macros is in #604. We can still merge this since the macros are better.

Crola1702 and others added 3 commits May 8, 2024 08:35
@Crola1702 @azeey
Update src/CMakeLists.txt
51926a6 
Co-authored-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Cristóbal Arroyo <[email protected]>
@Crola1702
Merge branch 'ign-common4' into Crola1702/5_to_4_25-04-2024
4b07733 
Signed-off-by: Cristóbal Arroyo <[email protected]>
@Crola1702
Remove extra space
6a20624 
Signed-off-by: Crola1702 <[email protected]>
@Crola1702 Crola1702 requested a review from azeey May 8, 2024 14:09
@Crola1702
Copy link
Contributor Author

I think everything is ready to go. Waiting for approvals and then merge

@azeey
Copy link
Contributor

azeey commented May 8, 2024

The examples are failing on CI. This also shows that we are changing the dependency on ign-utils from being a requirement of the graphics component to being a requirement for the core library. I'm not exactly sure why the example is failing, but I'm inclined to close this PR and live with the bare pragmas that were added in #603.

@Crola1702 Crola1702 closed this May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcoag marcoag Awaiting requested review from marcoag marcoag is a code owner

@scpeters scpeters Awaiting requested review from scpeters

@azeey azeey Awaiting requested review from azeey

Assignees
No one assigned
Labels
🌱 garden Ignition Garden 🎵 harmonic Gazebo Harmonic
Projects
Core development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

EnumIterator inherits from std::iterator base class which is deprecated in c++17
3 participants
@Crola1702 @scpeters @azeey