Unidata / netcdf-cxx4

Official GitHub repository for netCDF-C++ libraries and utilities.
Other
126 stars 49 forks source link

C++11 Move Constructors #25

Open citibeth opened 8 years ago

citibeth commented 8 years ago

When using C++11, move constructors are needed for NcFile and other relevant classes. The following should work, but doesn't.

class NcIO {
    netCDF::NcFile _mync;
public:

    NcIO(std::string const &filePath, netCDF::NcFile::FileMode fMode) :
        _mync(netCDF::NcFile(filePath, fMode)) {}
};
jarlela commented 8 years ago

I agree that move constructors would be a nice feature. However, your code example could easily be rewritten to work without them:

...
  NcIO(std::string const &filePath, netCDF::NcFile::FileMode fMode) :
      _mync(filePath, fMode) {}
};
citibeth commented 8 years ago

I should know better, that was embarrassing...

On Fri, Jan 8, 2016 at 3:37 AM, Jarle Ladstein notifications@github.com wrote:

I agree that move constructors would be a nice feature. However, your code example could easily be rewritten to work without them:

... NcIO(std::string const &filePath, netCDF::NcFile::FileMode fMode) : _mync(filePath, fMode) {} };

— Reply to this email directly or view it on GitHub https://github.com/Unidata/netcdf-cxx4/issues/25#issuecomment-169934714.

kain88-de commented 8 years ago

Move constructors would be very nice anyway to use it with the auto keyword

auto file = netcdf::ncFile(fname, netcdf::ncFile::read);

Currently this code fails because the copy constructor is private and no move constructor is generated.

htmlboss commented 6 years ago

I'd be quite happy to work on this feature if it's still wanted. I'm currently using this in a C++17 environment and this, among other modern c++ niceties, is near the top of my wishlist.

WardF commented 6 years ago

@htmlboss that would be fantastic, we'd welcome the contribution :).

htmlboss commented 6 years ago

Finally have some time to attack this...I see 2 ways of going about this thing:

  1. Test against the __cplusplus macro:
    #if __cplusplus >= 201103L
    #define HAS_CXX11
    #endif

    and then:

    
    #ifdef HAS_CXX11

// Move constructor NcFile(NcFile&&) = default;

// Move assignment NcFile& operator=(NcFile&&) = default;

endif


That's your move-semantics 😸 

This is really quick and easy, but some compilers may not adhere to correctly setting the `__cplusplus` macro, although it's defined in the c++ standard.

2. Doing some magic with autotools. Not really familiar with it but I quickly found out it can reliably test for specific c++ versions.

@WardF what's your opinion?
WardF commented 6 years ago

I'm concerned about accidentally boxing out some developers because of a compiler they're using, but I think most modern compilers support setting the __cplusplus macro. Lets go with option 1 since it should be the easiest way forward. If we receive a complaint, it should be easy enough for us to move the check into autotools and cmake; I found the cmake info here: https://stackoverflow.com/questions/10984442/how-to-detect-c11-support-of-a-compiler-with-cmake

Thanks again for your help with this!