inconsistent data member naming convention in pcl::PointCloud

[csukuangfj]

Your Environment

• PCL Version: the master branch

Context

See the code

pcl/common/include/pcl/point_cloud.h

Lines 413 to 423 in 9467b94

	uint32_t width;
	/** \brief The point cloud height (if organized as an image-structure). */
	uint32_t height;
	/** \brief True if no points are invalid (e.g., have NaN or Inf values in any of their floating point fields). */
	bool is_dense;
	/** \brief Sensor acquisition pose (origin/translation). */
	Eigen::Vector4f sensor_origin_;
	/** \brief Sensor acquisition pose (rotation). */
	Eigen::Quaternionf sensor_orientation_;

pcl/common/include/pcl/point_cloud.h

Line 592 in 9467b94

boost::shared_ptr<MsgFieldMap> mapping_;

Some data members are ended with _ and some are not.

Expected Behavior

Either all end with _ or none.

Current Behavior

The naming convention is not consistent.

Code to Reproduce

See the code above.

Possible Solution

Rename the data members consistently.

[taketwo]

You are right, there should be no _ suffix. However, I don't see a backwards-compatible way to perform such a rename. Do you have any ideas?

[SergioRAgostinho]

Here's a suggestion. http://cpp.sh/6yamj

[csukuangfj]

@SergioRAgostinho
Another solution might be

union
{
    int field;
    int field_;
};

which can handle the assignment operator and copy constructor automatically.

[SergioRAgostinho]

That's definitely a better strategy and performs similar.

@taketwo maybe we could even redo the same in #2102. Move that one to a simple deprecation.

[taketwo]

@csukuangfj great idea! And we can add a deprecation warning as well:

union
{
    int field;
    int field_ __attribute__ ((deprecated));
};

@SergioRAgostinho I'm not sure... do you think it's worth the effort?

[csukuangfj]

pcl/common/include/pcl/pcl_macros.h

Lines 324 to 337 in d1d59c6

	// check Intel compiler first since it usually also defines __GNUC__, __clang__, etc.
	#if defined(__INTEL_COMPILER)
	#define PCL_DEPRECATED(message) __attribute((deprecated))
	#elif (defined(__GNUC__) && PCL_LINEAR_VERSION(__GNUC__,__GNUC_MINOR__,__GNUC_PATCHLEVEL__) < PCL_LINEAR_VERSION(4,5,0) && ! defined(__clang__)) || defined(__INTEL_COMPILER)
	#define PCL_DEPRECATED(message) __attribute__ ((deprecated))
	// gcc supports this starting from 4.5 : http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43666
	#elif (defined(__GNUC__) && PCL_LINEAR_VERSION(__GNUC__,__GNUC_MINOR__,__GNUC_PATCHLEVEL__) >= PCL_LINEAR_VERSION(4,5,0)) || (defined(__clang__) && __has_extension(attribute_deprecated_with_message))
	#define PCL_DEPRECATED(message) __attribute__ ((deprecated(message)))
	#elif defined(_MSC_VER)
	#define PCL_DEPRECATED(message) __declspec(deprecated(message))
	#else
	#pragma message("WARNING: You need to implement PCL_DEPRECATED for this compiler")
	#define PCL_DEPRECATED(message)
	#endif

It's better use PCL_DEPRECATED() since it is compiler independent.

[stale]

Marking this as stale due to 30 days of inactivity. It will be closed in 7 days if no further activity occurs.

[kunaltyagi]

do you think it's worth the effort