Skip to content

Replace pcl_isnan, pcl_isfinite and pcl_isinf by C++11 methods#2798

Merged
SergioRAgostinho merged 5 commits intoPointCloudLibrary:masterfrom
SunBlack:pcl_macro_cleanup
Jan 30, 2019
Merged

Replace pcl_isnan, pcl_isfinite and pcl_isinf by C++11 methods#2798
SergioRAgostinho merged 5 commits intoPointCloudLibrary:masterfrom
SunBlack:pcl_macro_cleanup

Conversation

@SunBlack
Copy link
Copy Markdown
Contributor

No description provided.

@SunBlack
Copy link
Copy Markdown
Contributor Author

Compile issue is raised here:

template <> inline bool
isFinite<pcl::BorderDescription> (const pcl::BorderDescription &p)
{
return (pcl_isfinite (p.x) && pcl_isfinite (p.y));
}

Reason: p.x and p.y are of type int, but MS failed to implement bool isfinite( IntegralType arg ); correct.

I could simply change code to:

 template <> inline bool 
 isFinite<pcl::BorderDescription> (const pcl::BorderDescription &p) 
 { 
   return (std::isfinite (static_cast<double>(p.x)) && std::isfinite (static_cast<double>(p.y))); 
 } 

But this should always return true, because it is and int type and can't store and infinite value, or? So maybe this is better fix:

 template <> inline bool 
 isFinite<pcl::BorderDescription> (const pcl::BorderDescription &p) 
 { 
   return true; 
 } 

What do you think?

@taketwo
Copy link
Copy Markdown
Member

taketwo commented Jan 22, 2019

Sure, integer can not be infinite; that test makes no sense. Just return true, perhaps move the specialization so that it's a part of the group of "always true" tests.

@SunBlack
Copy link
Copy Markdown
Contributor Author

Found a lot of more senseless calls to these functions. Thanks for this compile issue MSVC to find this stupid calls 😄

Btw: I reordered template specialization lexicographically.

@SunBlack SunBlack force-pushed the pcl_macro_cleanup branch 5 times, most recently from 69eb66c to 1f363c7 Compare January 23, 2019 15:29
Copy link
Copy Markdown
Member

@taketwo taketwo left a comment

Choose a reason for hiding this comment

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

The last commit has too many unrelated changes in my opinion. Could you please split it while addressing comments?

@SunBlack
Copy link
Copy Markdown
Contributor Author

The last commit has too many unrelated changes in my opinion. Could you please split it while addressing comments?

They are not unrelated. This are all MSVC fixes (Azure failed before these changes).

@SunBlack SunBlack force-pushed the pcl_macro_cleanup branch 2 times, most recently from 6db1fce to d6b7134 Compare January 24, 2019 14:59
ThorstenHarter pushed a commit to ThorstenHarter/pcl that referenced this pull request Jan 24, 2019
@SunBlack SunBlack force-pushed the pcl_macro_cleanup branch 3 times, most recently from 2bf59eb to 1c03c36 Compare January 24, 2019 15:24
…VC compile issue due to missing IntegralType overloading.
@taketwo taketwo added the c++14 label Jan 30, 2019
Copy link
Copy Markdown
Member

@SergioRAgostinho SergioRAgostinho left a comment

Choose a reason for hiding this comment

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

As usual, thanks for the effort.

@SergioRAgostinho SergioRAgostinho merged commit b264eda into PointCloudLibrary:master Jan 30, 2019
@SunBlack SunBlack deleted the pcl_macro_cleanup branch January 30, 2019 12:27
@SergioRAgostinho SergioRAgostinho mentioned this pull request Sep 13, 2019
11 tasks
@SergioRAgostinho SergioRAgostinho added changelog: ABI break Meta-information for changelog generation and removed changelog: ABI break Meta-information for changelog generation labels Nov 8, 2019
lnexenl added a commit to lnexenl/PV-LIO that referenced this pull request Aug 25, 2023
 according to:
* **[modernization]** Deprecate `pcl_isnan`, `pcl_isfinite`, and `pcl_isinf` in favor of `std` methods [[#2798](PointCloudLibrary/pcl#2798), [#3457](PointCloudLibrary/pcl#3457)]
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.

3 participants