Skip to content

[GPU] Remove hand-rolled static_assert#4797

Merged
mvieth merged 5 commits intoPointCloudLibrary:masterfrom
FabianSchuetze:remove_old_assertions
Feb 12, 2022
Merged

[GPU] Remove hand-rolled static_assert#4797
mvieth merged 5 commits intoPointCloudLibrary:masterfrom
FabianSchuetze:remove_old_assertions

Conversation

@FabianSchuetze
Copy link
Copy Markdown
Contributor

The PR contains cosmetic changes to make the GPU code easier to read. The GPU code relies on template metaprogramming for a static check, but more concise c++11 functionality can replace these checks. Once PCL requires CUDA7 as the minimum CUDA version, we can remove the implementation of this static check altogether and replace the last remaining call with c++11 code.

@larshg
Copy link
Copy Markdown
Contributor

larshg commented Jun 9, 2021

find_package(CUDA 9.0)

Currently we search for version 9. Its also the minimum version on our CI. So I would say you can go ahead and remove it?

@FabianSchuetze
Copy link
Copy Markdown
Contributor Author

Thank you so much, @larshg, for pointing this out. It always fills me with join if I can remove unneeded code. Thanks to your comment, we can even remove an entire file.

@larshg
Copy link
Copy Markdown
Contributor

larshg commented Jun 10, 2021

* Author: Anatoly Baskeheev, Itseez Ltd, (myname.mysurname@mycompany.com)
*/

//TODO: Once PCL requires min CUDA7 (allows c++11 on device), remove file
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Instead of removing, let's deprecate the header for PCL 1.15 release

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.

Thanks for your comment! I added a deprecated attribute. Did you had this in mind? Is there another way to mark pending deprecation?

template<> struct Static<true>

template <>
struct [[deprecated("This class will be replaced at PCL release 1.15 by "
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please also add a deprecation for the file itself. PCL_DEPRECATED_HEADER

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.

Thanks for the comment, Kunal! I have added the deprecation notice. Is that what you had in mind?

@kunaltyagi kunaltyagi added the changelog: deprecation Meta-information for changelog generation label Jun 18, 2021
@kunaltyagi kunaltyagi added this to the pcl-1.12.0 milestone Jun 18, 2021
@kunaltyagi kunaltyagi modified the milestones: pcl-1.12.0, pcl-1.12.1 Jun 30, 2021
@kunaltyagi kunaltyagi changed the title remove hand-rolled static_assertion [GPU] Remove hand-rolled static_assert Jun 30, 2021
@mvieth
Copy link
Copy Markdown
Member

mvieth commented Nov 20, 2021

@FabianSchuetze Are you interested in continuing this pull request? Seems like it is almost done

@mvieth mvieth modified the milestones: pcl-1.12.1, pcl-1.13.0 Nov 22, 2021
@FabianSchuetze
Copy link
Copy Markdown
Contributor Author

Thanks for your message, @mvieth . I am happy to continue with this PR and I tried to address Kunal's suggestion. The build failed twice, do you maybe have an idea how that is related to the code changes?

@larshg
Copy link
Copy Markdown
Contributor

larshg commented Nov 26, 2021

Seems like and odd failing unit test with following error:

/Users/runner/work/1/s/test/search/test_organized.cpp:259: Failure
Expected: (pointDist) <= (searchRadius), actual: 0.275439 vs 0.275439

So I think we can just ignore it.

@larshg
Copy link
Copy Markdown
Contributor

larshg commented Nov 26, 2021

I'm getting the following errors:
error : expected a comma (the one-argument version of static_assert is not enabled in this mode)
or
error C2429: language feature 'terse static assert' requires compiler flag '/std:c++17'

Seems one-argument is first supported when c++17 is enabled on VS.

See also: https://stackoverflow.com/questions/65056159/i-dont-know-why-this-static-assert-code-doesnt-work

@mvieth
Copy link
Copy Markdown
Member

mvieth commented Nov 26, 2021

I restarted the test and it should pass now, but yes, otherwise we can ignore it.
Oh right, in C++11 the static_assert also needs a message string. Could you add one each @FabianSchuetze ? Can be something simple.
https://en.cppreference.com/w/cpp/language/static_assert

@mvieth
Copy link
Copy Markdown
Member

mvieth commented Dec 23, 2021

@FabianSchuetze Do you have time to continue this PR (add a simple message string for each static_assert)? Otherwise I might take over

Comment thread gpu/features/src/features.cpp Outdated
Comment thread gpu/features/src/features.cpp Outdated
Comment thread gpu/features/src/features.cpp Outdated
Comment thread gpu/features/src/features.cpp Outdated
Comment thread gpu/features/src/features.cpp Outdated
Comment thread gpu/features/src/features.cpp Outdated
Comment thread gpu/features/src/features.cpp Outdated
Comment thread gpu/octree/src/cuda/octree_builder.cu Outdated
Comment thread gpu/octree/src/octree.cpp Outdated
Comment thread gpu/octree/src/octree.cpp Outdated
@mvieth mvieth requested a review from larshg January 16, 2022 13:28
@mvieth mvieth requested a review from kunaltyagi February 8, 2022 14:10
Copy link
Copy Markdown
Member

@kunaltyagi kunaltyagi left a comment

Choose a reason for hiding this comment

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

🚀

@mvieth mvieth merged commit cc0f433 into PointCloudLibrary:master Feb 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog: deprecation Meta-information for changelog generation module: gpu

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants