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

[ign to gz] C++ namespaces #50

Closed
wants to merge 6 commits into from
Closed

[ign to gz] C++ namespaces #50

wants to merge 6 commits into from

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented Apr 12, 2022

🎉 New feature

Summary

Changes:

  • Change all namespaces from ignition to gz
  • Add alias so that ignition still works for users (tick-tock's tick)
  • Added a test to make sure ignition still works
  • While at it, I added missing inline version namespaces in a couple of places

Ideally we'd issue deprecation warnings when using ignition. Unfortunately, C++ doesn't allow this:

namespace [[deprecated]] ignition = gz;

Clang's warning is very clear:

.../include/ignition/utils/config.hh:24:11: error: attributes cannot be specified on namespace alias
namespace [[deprecated]] ignition = gz;
          ^

So we'll need to either find a workaround, or don't issue deprecation warnings for namespaces.

In any case, we should wait until most of the stack is already using gz before we turn on these warnings, otherwise they would hide other warnings.

Test it

See the added test. I also expect downstream libraries not to need any modifications because of this PR.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial - We'll add complete migration tutorials at the end
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • 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.

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

Ideally we'd issue deprecation warnings when using ignition. Unfortunately, C++ doesn't allow this:

I assume we will rename the headers and provide ignition/utils/* headers with #pragma deprecation warnings inside. I would suggest placing the namespace ignition = gz; statement inside one of these headers

include/ignition/utils/config.hh.in Show resolved Hide resolved
@azeey
Copy link
Contributor

azeey commented Apr 12, 2022

Can we use this trick?

namespace [[deprecated]] ignition {
  using namespace gz;
}

https://godbolt.org/z/4eYEco8qx

Signed-off-by: Louise Poubel <[email protected]>
@chapulina
Copy link
Contributor Author

Can we use this trick?

Brilliant! ✨ Works like a charm. I added it commented out in 5fee873 so we don't enable warnings just yet. It didn't work with IGN_DEPRECATED, so I used [[deprecated]].

I assume we will rename the headers and provide ignition/utils/* headers with #pragma deprecation warnings inside. I would suggest placing the namespace ignition = gz; statement inside one of these headers

I was thinking of adding symlinks so we don't duplicate the headers. But I'm open to suggestions about a different place to put the alias. I went for config.hh because that's common across all libraries and including it adds little overhead

@scpeters
Copy link
Member

I assume we will rename the headers and provide ignition/utils/* headers with #pragma deprecation warnings inside. I would suggest placing the namespace ignition = gz; statement inside one of these headers

I was thinking of adding symlinks so we don't duplicate the headers. But I'm open to suggestions about a different place to put the alias. I went for config.hh because that's common across all libraries and including it adds little overhead

I wouldn't suggest using identical headers in ignition/utils/*. I would have some deprecation warnings and then a #include <gz/utils/___.hh> statement, similar to the following:

@chapulina
Copy link
Contributor Author

I wouldn't suggest using identical headers in ignition/utils/*. I would have some deprecation warnings and then a #include <gz/utils/___.hh> statement, similar to the following:

Ah ok. I'm not sure I fully understand though, do you think we should keep each one of the existing headers, and inside each of them, include the gz equivalent? Or would it be just one master ignition header that includes all the gz headers?

That may work better than symlinks because it's better for cross-platform 🤔

@chapulina chapulina added 🌱 garden Ignition Garden ign to gz Renaming Ignition to Gazebo. labels Apr 13, 2022
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: methylDragon <[email protected]>
Signed-off-by: methylDragon <[email protected]>
@methylDragon
Copy link
Contributor

This was superceded by #61 because I had to do the namespace migration following the header migration.

@chapulina
Copy link
Contributor Author

Closing in favor of #61

@chapulina chapulina closed this May 17, 2022
@chapulina chapulina deleted the chapulina/2/gz_ns branch May 17, 2022 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden ign to gz Renaming Ignition to Gazebo.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants