fix build issues on Windows

kejxu commented 5 years ago

undefine NO_ERROR from winerror.h to resolve name collision with message type
use <ctime> on Windows

tfoote commented 5 years ago

In our past discussions of this we've talked about undefining and then later redefining it after we've used it. Do you think that's necessary/valuable?

kejxu commented 5 years ago

In our past discussions of this we've talked about undefining and then later redefining it after we've used it. Do you think that's necessary/valuable?

that would be optimal, but might turn out to be too much work in practice. the reason is that NO_ERROR (or something similar) in the Windows header is a macro so it has to be un-defined each time NO_ERROR is used. for example (tf2 has a similar issue), in https://github.com/ros/geometry2/blob/melodic-devel/tf2_ros/src/buffer_client.cpp#L109

if(result.error.error != result.error.NO_ERROR){
    // ...
}

has to become

#if defined(NO_ERROR)
#undef NO_ERROR
if(result.error.error != result.error.NO_ERROR){
// re-define NO_ERROR from winerror.h
#define NO_ERROR 0L
#endif
    // ...
}

and this has to happen not only when the header is included, but also everywhere it's used. seems to me would make the code vulnerable, hard to read, and hard to maintain. this is definitely good approach and feasible, but other developers consuming this package might not know that this is necessary.

other solutions:

minimize (or remove all) use of platform-dependent headers (e.g. winerror.h). yet in this case it also means to use something other than boost/winapi.h, this could take a long time to switch
update msg names, or in geometry, ErrorValues, to avoid such name collision. with a Windows building pipeline/CI (we're still working on it), this would not be too hard for new packages
update how messages are transmitted, so all look-up keys use string literals. because "NO_ERROR" will not be affected by macros. however, this is impossible as it would probably change the entire design of ROS =)

seanyen commented 5 years ago

@tfoote any feedbacks on this? Thanks!

seanyen commented 5 years ago

@tfoote Sorry to ping again. Any chance to take a look so we can move this PR forward?

seanyen commented 4 years ago

@sloretz @tfoote Happy new year. This is ready for review and merge. Hope we can move this forward for noetic and melodic.

ros / geometry

fix build issues on Windows #182