Review casts in PCL

[kunaltyagi]

Context

With the migration to index_t, PCL will soon have some unneeded casts which should be removed

Describe the solution you'd like

Each cast needs to be reviewed and removed if it's unneeded. A non-exhaustive list of such situations:

• assigning index_t to width
• assigning size_t to Indices

Additional context

This will require significant help from others since the scope is wide, but the issue is not hard at all. To get an idea of the scale, please check the occurrence of static_cast via:

grep '\s*static_cast' * -nrH | grep -v '<float>' | grep -v 'cast<\S*\*\s*>' | wc -l

Note: this doesn't count C style casts

(High effort for total change, low for effort needed for piece-meal contributions)

[gnawme]

Changing static_cast instances to use functional casts (e.g. static_cast<int>(foo) becomes int(foo) would make the code look much less verbose.

[kunaltyagi]

Functional casts aren't limited to static_cast and can call dynamic_cast. Ofc, this is ok for integral parameters, but for other types, this can unknowingly involve runtime penalty. Making the rule to use static_cast always removes overhead during code write, review and modification sessions

[gnawme]

The C++ standard says

The conversions performed by (4.1) a const_­cast, (4.2) a static_­cast, (4.3) a static_­cast followed by a const_­cast, (4.4) a reinterpret_­cast, or (4.5) a reinterpret_­cast followed by a const_­cast, can be performed using the cast notation of explicit type conversion.

dynamic_cast isn't one of the conversions that can be performed. I do prefer an explicit cast, I was just noting that a functional cast makes things less verbose (but is harder to search for).

static_cast can be tricky:

The same semantic restrictions and behaviors apply, with the exception that in performing a static_­cast in the following situations the conversion is valid even if the base class is inaccessible: (4.6) a pointer to an object of derived class type or an lvalue or rvalue of derived class type may be explicitly converted to a pointer or reference to an unambiguous base class type, respectively; (4.7) a pointer to member of derived class type may be explicitly converted to a pointer to member of an unambiguous non-virtual base class type; (4.8) a pointer to an object of an unambiguous non-virtual base class type, a glvalue of an unambiguous non-virtual base class type, or a pointer to member of an unambiguous non-virtual base class type may be explicitly converted to a pointer, a reference, or a pointer to member of a derived class type, respectively.

Some of these sound a lot like a dynamic_cast. I tend to only use static_cast in straightforward situations, though.

[gnawme]

@mvieth Is this issue still relevant? (Do you understand what it's asking for?)

[mvieth]

@mvieth Is this issue still relevant? (Do you understand what it's asking for?)

I'm not totally sure, probably things like this: https://github.com/PointCloudLibrary/pcl/blob/31564ba78816a2e3216ed008c394646d1892ac7c/filters/include/pcl/filters/impl/conditional_removal.hpp#L755

In general, it is nicer to have fewer casts of course, by using the same type for all variables that are compared with/assigned to each other.