-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CMake] Add option to choose pcl::index_t
while compiling
#4166
[CMake] Add option to choose pcl::index_t
while compiling
#4166
Conversation
pcl_config.h.in
Outdated
|
||
#cmakedefine HAVE_TBB 1 | ||
|
||
#cmakedefine HAVE_OPENNI 1 | ||
|
||
#cmakedefine HAVE_OPENNI2 1 | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unneeded?
cmake/pcl_options.cmake
Outdated
@@ -64,3 +64,9 @@ set_property(GLOBAL PROPERTY USE_FOLDERS ON) | |||
option(BUILD_tools "Useful PCL-based command line tools" ON) | |||
|
|||
option(WITH_DOCS "Build doxygen documentation" OFF) | |||
|
|||
# set index size | |||
set(PCL_SIZE_INDEX 32 CACHE STRING "Set index size. Available options are: 8 16 32 64") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please read the note in types.h
regarding the default size. It can't be 32 for PCL 1.11 due to compatibility. It can be 32 for PCL 1.12 since the user can then override the size at compile time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would modifying types.h
to be
#if PCL_MINOR_VERSION <= 11
// sizeof returns bytes, while we measure size by bits in the template
#undef PCL_INDEX_SIZE
#define PCL_INDEX_SIZE (sizeof(int) * 8)
#else
#ifndef PCL_INDEX_SIZE
#define PCL_INDEX_SIZE 32
#endif // PCL_INDEX_SIZE
#endif // PCL_MINOR_VERSION
be an option or should the changes be made solely in the cmake configuration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can modify anything as long as it keeps the behavior (and is sane) 😄
Maybe squash the commits to a single commit :) ? |
@kunaltyagi I moved the version check for changing indices type to the config file, so that all of the configuration happens in one place. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, except inline
// sizeof returns bytes, while we measure size by bits in the template | ||
#define PCL_INDEX_SIZE (sizeof(int) * 8) | ||
#if (PCL_INDEX_SIZE_TEMP > 0) | ||
#define PCL_INDEX_SIZE ${PCL_INDEX_SIZE} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using PCL_INDEX_SIZE_TEMP
might be better instead of ${PCL_INDEX_SIZE}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like I can't follow both of these suggestions together. If I undefine PCL_INDEX_SIZE_TEMP
, then at compile time PCL_INDEX_SIZE
cannot be set to it, because it is no longer available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yeah. Can we just remove the temporary variable in that case? It's sole use seems to be comparison with 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unresolving the conversation, because PCL_INDEX_SIZE_TEMP
still needs to be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, I thought by remove @kunaltyagi meant I should undef PCL_INDEX_SIZE_TEMP
, not get rid of it altogether.
Is this what you had in mind?
#if (${PCL_INDEX_SIZE} > 0)
#define PCL_INDEX_SIZE ${PCL_INDEX_SIZE}
#else
#if PCL_MINOR_VERSION <= 11
// sizeof returns bytes, while we measure size by bits in the template
#define PCL_INDEX_SIZE (sizeof(int) * 8)
#else
#define PCL_INDEX_SIZE 32
#endif //PCL_MINOR_VERSION
#endif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I interpreted, because there's no need to actually define anything.
Co-authored-by: Sérgio Agostinho <sergio.r.agostinho@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
pcl::index_t
while compiling
Add cmake configuration to change indices type and sign.