Skip to content

Fix unsafe globals in Interval and Region3#396

Merged
chapulina merged 3 commits intogazebosim:mainfrom
hidmic:fix-unsafe-constant-initializations
Mar 30, 2022
Merged

Fix unsafe globals in Interval and Region3#396
chapulina merged 3 commits intogazebosim:mainfrom
hidmic:fix-unsafe-constant-initializations

Conversation

@hidmic
Copy link
Copy Markdown
Contributor

@hidmic hidmic commented Mar 25, 2022

🦟 Bug fix

Relates to #269.

Summary

This PR fixes a potential global initialization order issue introduced in #388 and #390 by making all Interval and Region3 construction forms constexpr.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)

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.

hidmic added 2 commits March 25, 2022 16:17
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic added the MBARI-LRAUV Sponsored by MBARI-LRAUV project: https://github.com/osrf/lrauv label Mar 25, 2022
@hidmic hidmic requested a review from chapulina March 25, 2022 19:22
@hidmic hidmic requested a review from scpeters as a code owner March 25, 2022 19:22
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Mar 25, 2022
@hidmic hidmic changed the title Fix unsafe constant initializations in Interval and Region3 Fix unsafe globals in Interval and Region3 Mar 25, 2022
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 25, 2022

Codecov Report

Merging #396 (e9095d1) into main (dcde2c0) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #396   +/-   ##
=======================================
  Coverage   99.73%   99.73%           
=======================================
  Files          69       69           
  Lines        6353     6353           
=======================================
  Hits         6336     6336           
  Misses         17       17           
Impacted Files Coverage Δ
include/ignition/math/Interval.hh 100.00% <100.00%> (ø)
include/ignition/math/Region3.hh 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dcde2c0...e9095d1. Read the comment docs.

Copy link
Copy Markdown
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

LGTM, but I'd like to make sure @scpeters approves too before merging

@scpeters
Copy link
Copy Markdown
Member

it seems right to me, but I think @azeey understands this issue better than me. I think the right way to test this is with asan using gazebosim/gz-cmake#210, but I haven't had a chance to do so. I think it's fine to merge without my approval. Once we get CI with sanitizers, we'll know for sure :)

@chapulina
Copy link
Copy Markdown
Contributor

Sounds reasonable, we can revisit when the sanitizers are running in CI, if needed

@chapulina chapulina merged commit 120806b into gazebosim:main Mar 30, 2022
@hidmic hidmic deleted the fix-unsafe-constant-initializations branch March 31, 2022 11:06
ahcorde pushed a commit that referenced this pull request Jul 4, 2022
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
chapulina pushed a commit that referenced this pull request Jul 8, 2022
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🌱 garden Ignition Garden MBARI-LRAUV Sponsored by MBARI-LRAUV project: https://github.com/osrf/lrauv

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants