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 some windows warnings (C4244 and C4305) #1874

Merged
merged 8 commits into from
Feb 3, 2023

Conversation

Crola1702
Copy link
Contributor

🦟 Bug fix

Fixes part of #1870

Summary

This PR fixes some warnings (C4244 and C4305) happening in ign-gazebo6 windows
Reference build: https://build.osrfoundation.org/job/ign_gazebo-ign-6-win/166/

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.

@Crola1702 Crola1702 requested a review from Blast545 January 30, 2023 13:36
@Crola1702 Crola1702 requested a review from mjcarroll as a code owner January 30, 2023 13:36
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Jan 30, 2023
@Crola1702 Crola1702 requested a review from azeey January 31, 2023 14:53
@codecov
Copy link

codecov bot commented Jan 31, 2023

Codecov Report

Merging #1874 (0a99678) into ign-gazebo6 (68008d2) will decrease coverage by 0.02%.
The diff coverage is 35.71%.

@@               Coverage Diff               @@
##           ign-gazebo6    #1874      +/-   ##
===============================================
- Coverage        64.80%   64.78%   -0.02%     
===============================================
  Files              322      322              
  Lines            26389    26391       +2     
===============================================
- Hits             17101    17098       -3     
- Misses            9288     9293       +5     
Impacted Files Coverage Δ
.../gui/plugins/transform_control/TransformControl.cc 7.36% <0.00%> (+0.01%) ⬆️
...rc/systems/ackermann_steering/AckermannSteering.cc 85.50% <71.42%> (-0.17%) ⬇️
src/SimulationRunner.cc 90.82% <0.00%> (-0.95%) ⬇️
src/Util.cc 93.44% <0.00%> (+0.32%) ⬆️

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

@azeey
Copy link
Contributor

azeey commented Jan 31, 2023

signoffs don't work correctly when applying code suggestions, so you'll have to follow the instructions here to rebase your commits with signoffs to fix the DCO error.

Crola1702 and others added 7 commits January 31, 2023 15:58
Signed-off-by: Crola1702 <cristobal.arroyo@ekumenlabs.com>
Signed-off-by: Crola1702 <cristobal.arroyo@ekumenlabs.com>
Signed-off-by: Crola1702 <cristobal.arroyo@ekumenlabs.com>
Signed-off-by: Crola1702 <cristobal.arroyo@ekumenlabs.com>
Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Crola1702 <cristobal.arroyo@ekumenlabs.com>
Signed-off-by: Crola1702 <cristobal.arroyo@ekumenlabs.com>
Signed-off-by: Crola1702 <cristobal.arroyo@ekumenlabs.com>
@Crola1702 Crola1702 force-pushed the Crola1702/fix-windows-warnings branch from b1346de to 5d27b81 Compare January 31, 2023 20:58
More details: 0a84abe

Signed-off-by: Crola1702 <cristobal.arroyo@ekumenlabs.com>
@mjcarroll
Copy link
Contributor

@osrf-jenkins retest this please

Copy link
Contributor

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

LGTM, just running through CI one more time.

@Crola1702
Copy link
Contributor Author

FYI: Commit 0a99678 fixes optical_tactile_plugin.cc test. I did kind of a backport from 0a84abe.

@Crola1702
Copy link
Contributor Author

Homebrew test regressions are known issues reported here: #1867

@Crola1702 Crola1702 merged commit 0960498 into ign-gazebo6 Feb 3, 2023
@Crola1702 Crola1702 deleted the Crola1702/fix-windows-warnings branch February 3, 2023 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants