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

Don't include the gz/math.hh header from library code #1043

Merged
merged 1 commit into from
Jun 7, 2022

Conversation

jwnimmer-tri
Copy link
Contributor

@jwnimmer-tri jwnimmer-tri commented Jun 7, 2022

🦟 Bug fix

Summary

Don't include the gz/math.hh header from library code. Using overly-broad include statements leads to slower builds and bloated object code. This commit swaps it out the gz/math.hh "everything" header for more targets files, instead.

Checklist

  • Signed all commits for DCO
  • Added tests (n/a)
  • Updated documentation (n/a)
  • Updated migration guide (n/a)
  • Consider updating Python bindings (n/a)
  • 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.

@github-actions github-actions bot added the 🌱 garden Ignition Garden label Jun 7, 2022
@jwnimmer-tri

This comment was marked as outdated.

Using overly-broad include statements leads to slower builds and
bloated object code.

Signed-off-by: Jeremy Nimmer <[email protected]>
@jwnimmer-tri jwnimmer-tri force-pushed the gz-math-specific-includes branch from bebcb4f to cadbd0f Compare June 7, 2022 14:44
@codecov
Copy link

codecov bot commented Jun 7, 2022

Codecov Report

Merging #1043 (cadbd0f) into main (ecb067f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1043   +/-   ##
=======================================
  Coverage   83.64%   83.64%           
=======================================
  Files         146      146           
  Lines       18255    18255           
=======================================
  Hits        15269    15269           
  Misses       2986     2986           
Impacted Files Coverage Δ
include/sdf/Param.hh 83.75% <ø> (ø)
src/parser_urdf.cc 82.99% <ø> (ø)

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 ecb067f...cadbd0f. Read the comment docs.

Copy link
Collaborator

@azeey azeey left a comment

Choose a reason for hiding this comment

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

Thanks! We've just been talking internally this week about how we can prevent usage of these type of headers in our libraries.

@azeey azeey merged commit f3457ef into gazebosim:main Jun 7, 2022
@jwnimmer-tri jwnimmer-tri deleted the gz-math-specific-includes branch June 7, 2022 15:58
azeey pushed a commit to azeey/sdformat that referenced this pull request Jun 7, 2022
Using overly-broad include statements leads to slower builds and
bloated object code.

Signed-off-by: Jeremy Nimmer <[email protected]>
azeey added a commit to azeey/gz-sim that referenced this pull request Jun 8, 2022
Some libsdformat headers used to include `gz/math.hh`, but that is no
longer the case since gazebosim/sdformat#1043.
As a result, gz-sim has to be updated to include the necessary headers.

Signed-off-by: Addisu Z. Taddese <[email protected]>
chapulina pushed a commit to gazebosim/gz-sim that referenced this pull request Jun 8, 2022
Some libsdformat headers used to include `gz/math.hh`, but that is no
longer the case since gazebosim/sdformat#1043.
As a result, gz-sim has to be updated to include the necessary headers.

Signed-off-by: Addisu Z. Taddese <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants