PointCloudLibrary / pcl

Point Cloud Library (PCL)
https://pointclouds.org/
Other
9.83k stars 4.6k forks source link

WIN32 instead of _WIN32 used in PCL #3670

Closed kunaltyagi closed 4 years ago

kunaltyagi commented 4 years ago

Your Environment

Context

Accidentally WIN32 and WIN64 has been used for Windows OS detection instead of _WIN32 and _WIN64 (notice the underscore)

Possible Solution

FSund commented 4 years ago

I just noticed there are also some WIN64 instances, like here: https://github.com/PointCloudLibrary/pcl/blob/master/surface/include/pcl/surface/3rdparty/opennurbs/opennurbs_system.h#L135

Are those valid?

That file also seems to indicate that windows.h has nested includes that define WIN32: https://github.com/PointCloudLibrary/pcl/blob/master/surface/include/pcl/surface/3rdparty/opennurbs/opennurbs_system.h#L142

FSund commented 4 years ago

I found an instance of

#ifndef WIN32
#define WIN32
#endif

in minwindefs.h in the headers that come with mingw, so it does seem like both _WIN32 and WIN32 might appear on Windows platforms.

kunaltyagi commented 4 years ago

I'm not a windows user, but from an implementor's PoV, I'd not recommend to keep the un-underscored versions. Maybe someone else can chime in with the recommended course of action.

larshg commented 4 years ago

WIN32 is added manually to the [default CXX flag] (https://github.com/PointCloudLibrary/pcl/blob/fe10b7619db9a89831b5ad418a265272ea15581e/CMakeLists.txt#L88)s if compiler is MSCV.

And atleast here it ends up in the definitions:

image

Not sure if it applies to MSYS2/Mingw as @FSund uses?

FSund commented 4 years ago

CMAKE_COMPILER_IS_MSVC is false for mingw, so I don't think I would have `WIN32 defined from there. That might be the reason for the issue I had in #3667.

stale[bot] commented 4 years ago

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