Add pcl::index_t; move some type declarations from pcl/pcl_macros.h to pcl/types.h

[kunaltyagi]

tl;dr

A new type pcl::index_t is introduced. This type will be used for pointcloud indexing in the future.

Breaking change: some type declarations (e.g. pcl::uint8_t) we extracted from pcl/pcl_macros.h to a new header file pcl/types.h. In most cases, the users of these types do not need to do anything because pcl/types.h will be transitively included by other PCL headers.

---

• Separated types from macros
• Added pcl::index_t and related changes

TODO (need to discuss where):

• CMake integration for PCL_INDEX_SIZE and PCL_INDEX_SIGNED. Might make sense to have a separate PR for this and keep this strictly related to new type
• Use pcl::index_t in Indices

[kunaltyagi]

Can we change the name of point_traits.h to type_traits.h? Will this be a breaking change? if so, should we have a redirect to the new name?

[mvieth]

As I have explained here, I would make unsigned indices the default, not signed ones.

[kunaltyagi]

The current usage is int which is signed. I'm not "breaking" anything but adding functionality to allow users to pick their own poison. Breaking change in this version will be bad. We can shift after adding the type, but not before providing the users an "out"

Nothing against your argument, simply there needs to be a roadmap to the final change, and no matter what the target is, I feel this is the road to take:

1. add a type
2. convert all internal usage to type
3. make it configurable with CMake
4. break whatever rule needed

[mvieth]

Makes sense, but wouldn't it already be "breaking" when switching from int to std::int32_t?

[kunaltyagi]

Good point. I'd have to revisit the code and maintain compatibility for this release. Thanks for noticing that

[SergioRAgostinho]

• CMake integration for PCL_INDEX_SIZE and PCL_INDEX_SIGNED. Might make sense to have a separate PR for this and keep this strictly related to new type

Agreed

Can we change the name of point_traits.h to type_traits.h? Will this be a breaking change? if so, should we have a redirect to the new name?

At first sight yes, yes and yes.

[SergioRAgostinho]

LGTM. You're moving the int types from pcl_macros.h to pcl_types.h. This will break consumer code.

Do we need to call the new header pcl_types.h, when it already resides in <pcl/....>. How about <pcl/types.h>?

[kunaltyagi]

Do we need to call the new header pcl_types.h

Nope. Just following the names of other files. pcl/types.h seems better

This will break consumer code

That's unfortunate, but I don't see a way around that. The breakage can be prevented by breaking pcl_macros in pieces and including the newly created headers with a #warning in the new pcl_macros

[kunaltyagi]

We have some similar looking meta-programming on integer types in type_traits.h and types.h

[SergioRAgostinho]

We have some similar looking meta-programming on integer types in type_traits.h and types.h

Do you mean <type_traits>? Otherwise I don't know which file you're referring to.

[kunaltyagi]

I mean the two parallel PRs, this one and the one about point_traits.h->type_traits.h

Not the same logic about integer types, but similar.

[SergioRAgostinho]

I'm not really sure what feedback is still missing but feel free to merge from my side.

[kunaltyagi]

Oh none. It was added quite early on and just didn't get removed

Waiting for the CI before merge

[taketwo]

Do I understand right that "API break" refers to the fact that some types were moved out of pcl_macros.h into a new file?

[kunaltyagi]

Yes

[taketwo]

OK. I've updated the title and the PR description to provide more insight.

Remember that PRs that end up in the "Notable changes" section of the changelog (and especially the "API break" ones) are likely to be noticed/read by our users. I imagine the following process:

1. "API break" label catches readers attention.
2. The title provides a one-sentence summary of what is broken. Readers decide whether they are interested. If yes, then they click on the PR link.
3. PR description provides a more detailed summary of the changes and suggestions what the users should do about it in their code.

Taking this PR as an example, the "API break" label is confusing given that the title is simply "Adding configurable pcl::index_t". How can the addition of a type be breaking? The confused reader then clicks on the PR link, but there is still no clear explanation of what was broken and how to transition.

Sidenote: this is another reason why we prefer granular PRs. If the PR title for the changelog needs to contain two clauses joined by a semicolon, then these probably should have been two PRs in the first place.

[mvieth]

So what are the next steps after index_t is introduced? When/for what release should index_t be used in the code and replace e.g. int?

[kunaltyagi]

I'll be trying for 1.11 (replacing int with index_t)

[mvieth]

@kunaltyagi I think I found an error (a bit late, I know, sorry).
Here you seem to assume that sizeof returns bits, but it returns bytes, so likely 4 or 8 for int. So instead of 8, 16, 32, 64 you would have to use 1, 2, 4, 8 later.

[kunaltyagi]

I think I recently sent a patch for that, included in #3822

[mvieth]

Ah yes, that should work. I didn't see that, I just noticed the error on the current master branch.