jhu-cisst / cisst

JHU ERC CISST Library
http://github.com/jhu-cisst/cisst/wiki
Other
64 stars 47 forks source link

Cleaned up use of virtual/override #98

Closed adeguet1 closed 3 months ago

adeguet1 commented 11 months ago

see #96

pkazanzides commented 11 months ago

Seems to mostly work on Windows with VS2017. But, I am getting some errors when compiling the Python wrappers, with lines such as double Scalar(const size_t index) const CISST_THROW(std::out_of_range) override; The compiler message isn't very helpful, it just says "Syntax error - possibly a missing semicolon". I think it might be the placement of the CISST_THROW macro. Have you tried compiling the Python wrappers on Linux?

adeguet1 commented 11 months ago

The code compile on Linux, including the Python wrappers (on by default when building for ROS 1 with catkin). I wonder if the definition of CISST_THROW in cmnPortability.h is correct.

pkazanzides commented 11 months ago

This seems to be an issue with SWIG. I was using version 3.0.12. It works fine with the latest version (4.1.1). Perhaps we should update the minimum requirement for SWIG, which curiously seems to be significantly out of date here.

If we want to support older versions of SWIG, I found that following could be added to cmnPortability.h:

#ifdef SWIG
#define override
#endif
adeguet1 commented 3 months ago

The default swig version on Ubuntu 20.04 is 4.0.1 and this version supports the override. On Ubuntu 18.04, swig version is 3.0.12. I'm not sure how long we will support 18.04 but setting the minimum version in find_package would prevent compiling the Python wrappers on older computers. Maybe we should used SWIG_VERSION in cmnPortability (see https://www.swig.org/Doc4.1/SWIGDocumentation.html#Preface_nn9): ``

ifdef SWIG_VERSION

if SWIG_VERSION < 0x040000

define override

endif

endif

pkazanzides commented 3 months ago

Checking SWIG_VERSION in cmnPortability.h seems like a good idea to me.