Skip to content

Use isnan in DepthImageOctomapUpdater#2201

Merged
tylerjw merged 1 commit intomoveit:mainfrom
EzraBrooks:use-isnan-in-depth-image-octomap-updater
May 23, 2023
Merged

Use isnan in DepthImageOctomapUpdater#2201
tylerjw merged 1 commit intomoveit:mainfrom
EzraBrooks:use-isnan-in-depth-image-octomap-updater

Conversation

@EzraBrooks
Copy link
Copy Markdown
Member

Description

Uses std::isnanfor a particularly nasty conditional we saw in the Sonar scan that haunted me, instead of relying on the fact that NaN != NaN.

Copy link
Copy Markdown
Contributor

@ChrisThrasher ChrisThrasher left a comment

Choose a reason for hiding this comment

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

Common SonarCloud W


// if there are any NaNs, discard data
if (!(px == px && py == py && inv_fx_ == inv_fx_ && inv_fy_ == inv_fy_))
if (isnan(px) || isnan(py) || isnan(inv_fx_) || isnan(inv_fy_))
Copy link
Copy Markdown
Contributor

@ChrisThrasher ChrisThrasher May 23, 2023

Choose a reason for hiding this comment

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

isnan() from the C standard library is actually an implementation-defined macro where as std::isnan() from the C++ standard library is a proper function and overloaded on all floating point types. I doubt the difference matters but I wanted to point that out.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

huh, I was confused by exactly that cppreference page

Instead of relying on the fact that `NaN != NaN`.
@tylerjw tylerjw force-pushed the use-isnan-in-depth-image-octomap-updater branch from 0d8a391 to 6cc45f4 Compare May 23, 2023 17:18
@tylerjw tylerjw enabled auto-merge (squash) May 23, 2023 17:18
@codecov
Copy link
Copy Markdown

codecov bot commented May 23, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.01 🎉

Comparison is base (21036b5) 50.56% compared to head (0d8a391) 50.56%.

❗ Current head 0d8a391 differs from pull request most recent head 6cc45f4. Consider uploading reports for the commit 6cc45f4 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2201      +/-   ##
==========================================
+ Coverage   50.56%   50.56%   +0.01%     
==========================================
  Files         387      387              
  Lines       31740    31740              
==========================================
+ Hits        16045    16047       +2     
+ Misses      15695    15693       -2     

see 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tylerjw tylerjw merged commit 052c217 into moveit:main May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants