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

Clean up library.hh headers #479

Merged
merged 3 commits into from
Aug 30, 2022
Merged

Clean up library.hh headers #479

merged 3 commits into from
Aug 30, 2022

Conversation

chapulina
Copy link
Contributor

🦟 Bug fix

Summary

  • Include strictly necessary headers instead of library.hh.
  • Remove Windows warning suppressions that aren't needed anymore.

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.

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

Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina chapulina added bug Something isn't working 🌱 garden Ignition Garden labels Aug 26, 2022
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@codecov
Copy link

codecov bot commented Aug 26, 2022

Codecov Report

Merging #479 (d7aaecf) into gz-gui7 (b5f354a) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           gz-gui7     #479   +/-   ##
========================================
  Coverage    67.82%   67.82%           
========================================
  Files           44       44           
  Lines         4826     4826           
========================================
  Hits          3273     3273           
  Misses        1553     1553           
Impacted Files Coverage Δ
src/plugins/camera_tracking/CameraTracking.cc 68.45% <ø> (ø)
src/plugins/grid_config/GridConfig.cc 22.35% <ø> (ø)
src/plugins/grid_config/GridConfig.hh 100.00% <ø> (ø)
src/plugins/image_display/ImageDisplay.cc 31.70% <ø> (ø)
...interactive_view_control/InteractiveViewControl.cc 17.88% <ø> (ø)
src/plugins/key_publisher/KeyPublisher.cc 56.52% <ø> (ø)
src/plugins/marker_manager/MarkerManager.cc 59.41% <ø> (ø)
src/plugins/minimal_scene/MinimalScene.cc 65.61% <ø> (ø)
src/plugins/publisher/Publisher.cc 90.78% <ø> (ø)
src/plugins/screenshot/Screenshot.cc 30.12% <ø> (ø)
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@chapulina
Copy link
Contributor Author

It looks like there are some Windows warnings still leaking through moc_TopicEcho.cpp, even though they should be suppressed by

#ifdef _MSC_VER
#pragma warning(push, 0)
#endif
#include <google/protobuf/message.h>
#ifdef _MSC_VER
#pragma warning(pop)
#endif

🤔

@mjcarroll
Copy link
Contributor

I ran into this somewhere else, I don't think that #pragma warning(push, 0) is valid...

From the docs

The warning pragma also supports the following syntax, where the optional n parameter represents a warning level (1 through 4).

@jennuine
Copy link
Contributor

It looks like there are some Windows warnings still leaking through moc_TopicEcho.cpp, even though they should be suppressed

Would they be fixed if we added:

#pragma warning(disable: 4251)

?

@mjcarroll
Copy link
Contributor

Would they be fixed if we added:

Yes, this is the correct mechanism, it's what I had to do elsewhere.

Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
Copy link
Contributor Author

Would they be fixed if we added:

Done in d7aaecf, hopefully Windows CI will be green now 🤞🏽

@chapulina chapulina merged commit d32c41b into gz-gui7 Aug 30, 2022
@chapulina chapulina deleted the chapulina/msgs.hh branch August 30, 2022 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 🌱 garden Ignition Garden
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants