Skip to content

Fix clang warnings.#267

Merged
scpeters merged 1 commit intogazebosim:ign-math6from
clalancette:clalancette/fix-clang-warnings
Oct 14, 2021
Merged

Fix clang warnings.#267
scpeters merged 1 commit intogazebosim:ign-math6from
clalancette:clalancette/fix-clang-warnings

Conversation

@clalancette
Copy link
Copy Markdown
Contributor

When building Material.cc with clang, it warns that the loop
construct used is a reference, but a copy is going to be made
anyway because of a const/non-const mismatch. Fix this by
adding const to the loop variable type, which removes the
warning and should be more efficient.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

🦟 Bug fix

Fixes #

Summary

Fixes warnings that come when building ign-math6 with clang. See https://ci.ros2.org/view/nightly/job/nightly_linux_clang_libcxx/940/clang/new/ for the warnings themselves.

Checklist

  • Signed all commits for DCO
  • Added tests (no new tests needed, compilation warning only)
  • Updated documentation (as needed) (not needed)
  • Updated migration guide (as needed) (not 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

When building Material.cc with clang, it warns that the loop
construct used is a reference, but a copy is going to be made
anyway because of a const/non-const mismatch.  Fix this by
adding const to the loop variable type, which removes the
warning and should be more efficient.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette clalancette requested a review from scpeters as a code owner October 14, 2021 13:44
@github-actions github-actions bot added Gazebo 1️1️ Dependency of Gazebo classic version 11 🌱 garden Ignition Garden 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome labels Oct 14, 2021
@codecov
Copy link
Copy Markdown

codecov bot commented Oct 14, 2021

Codecov Report

Merging #267 (db74d64) into ign-math6 (84eb1bf) will not change coverage.
The diff coverage is 100.00%.

❗ Current head db74d64 differs from pull request most recent head 0263890. Consider uploading reports for the commit 0263890 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           ign-math6     #267   +/-   ##
==========================================
  Coverage      99.41%   99.41%           
==========================================
  Files             67       67           
  Lines           6374     6374           
==========================================
  Hits            6337     6337           
  Misses            37       37           
Impacted Files Coverage Δ
src/Material.cc 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 84eb1bf...0263890. Read the comment docs.

@scpeters scpeters merged commit bf4afb6 into gazebosim:ign-math6 Oct 14, 2021
@clalancette
Copy link
Copy Markdown
Contributor Author

Thank you! Any idea when a new tag/release will be made here? I'd like to update https://github.com/ignition-release/ignition_math6_vendor with a tag that contains this fix. Alternatively, I can switch the vendor package to use a git hash number instead.

@chapulina
Copy link
Copy Markdown
Contributor

Any idea when a new tag/release will be made here?

We can make one now: #268

@clalancette clalancette deleted the clalancette/fix-clang-warnings branch October 14, 2021 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏰 citadel Ignition Citadel 🔮 dome Ignition Dome 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress 🌱 garden Ignition Garden Gazebo 1️1️ Dependency of Gazebo classic version 11

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants