Skip to content

[all] Fix compilation warnings - #6226

Open
hugtalbot wants to merge 3 commits into
masterfrom
202607_fix_warnings_claude
Open

[all] Fix compilation warnings#6226
hugtalbot wants to merge 3 commits into
masterfrom
202607_fix_warnings_claude

Conversation

@hugtalbot

@hugtalbot hugtalbot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Massively fix warnings for the pleasure

What was fixed

  • Unused parameters
  • Sign-compare
  • Deprecated barycentric API for you @epernod

Some points worth attention

  • BoxROI.inl was a genuine out-of-bounds read, not just a warning. isPointInAlignedBox looped to DataTypes::spatial_dimensions and indexed box[i+3], so for Vec6Types it read indices 6/7/8 of a Vec6. An axis-aligned box packed as (min₃, max₃) can't describe more than 3 dimensions, so I capped the loop at 3 and documented that higher dimensions are ignored.
  • CCDTightInclusionIntersection.cpp:187 — the bool from ticcd::edgeEdgeCCD is discarded and a contact is created unconditionally. That looks like a real bug, but acting on it changes collision behaviour, so I only silenced it ([[maybe_unused]] + a comment noting the call is kept for its toi/outputTolerance outputs). Your call whether it should early-return.

By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).


Reviewers will merge this pull-request only if

  • it builds with SUCCESS for all platforms on the CI.
  • it does not generate new warnings.
  • it does not generate new unit test failures.
  • it does not generate new scene test failures.
  • it does not break API compatibility.
  • it is more than 1 week old (or has fast-merge label).

@hugtalbot
hugtalbot requested a review from epernod August 11, 2026 16:53
@hugtalbot hugtalbot added pr: fix Fix a bug pr: status to review To notify reviewers to review this pull-request pr: AI-aided Label notifying the reviewers that part or all of the PR has been generated with the help of an AI labels Aug 11, 2026
@hugtalbot

Copy link
Copy Markdown
Contributor Author

[ci-build][with-all-tests]

Comment on lines +196 to +197
static constexpr typename type::Vec6::size_type nbTestedDimensions =
DataTypes::spatial_dimensions < 3 ? DataTypes::spatial_dimensions : 3;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
static constexpr typename type::Vec6::size_type nbTestedDimensions =
DataTypes::spatial_dimensions < 3 ? DataTypes::spatial_dimensions : 3;
static constexpr typename type::Vec6::size_type nbTestedDimensions = std::min(DataTypes::spatial_dimensions, 3);

{
const auto& elements = trait::FiniteElement::getElementSequence(*this->l_topology);
// makes sure the element sequence is available in the topology container
[[maybe_unused]] const auto& elements = trait::FiniteElement::getElementSequence(*this->l_topology);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

not sure about this one

@bakpaul

bakpaul commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

I confirm that in the collision pipeline the collision if first tested before the projection is done. So whenever we enter this function we know for sure that there is a collision, so no need to check that bool.

Comment on lines +187 to +189
// the returned boolean is not tested here: computeIntersection is only called on pairs for which
// testIntersection already reported a collision. The call is kept for its outputs: toi and outputTolerance.
[[maybe_unused]] const auto result = ticcd::edgeEdgeCCD(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I guess you are suggesting to removing this then @bakpaul

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: AI-aided Label notifying the reviewers that part or all of the PR has been generated with the help of an AI pr: fix Fix a bug pr: status to review To notify reviewers to review this pull-request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants