`noexcept` on destructor causing MSVC ICE

[lightmare]

https://github.com/mapnik/mapnik/issues/3277#issuecomment-177471394

I will send a pull request commenting 24f69e1e9586579e8020baea02e9cf2f7d23380e out for MSC. If there's something wrong with either std::is_nothrow_destructible or default exception-specification on destructors in MSVC, I think having none is still better than having an ICE-ing one :icecream:

But this needs a test case that MSVC won't compile (with noexcept on destructor). So far it seems it needs to involve:

struct with_user_declared_dtor
{
    ~with_user_declared_dtor() // noexcept(true) by default
    {}
};

struct with_virtual_dtor
{
    virtual ~with_virtual_dtor() // noexcept(true) by default
    {}
};

struct with_variant_member_and_dtor : with_virtual_dtor
{
    variant<int, with_user_declared_dtor> var;
    virtual ~with_variant_member_and_dtor() // noexcept(true) by default
    // but MSVC somehow concludes this dtor is noexcept(false),
    // probably because is_nothrow_destructible<tuple<int, with_user_declared_dtor>>
    // returns false (which is wrong, imo)
    {}
};

Can someone test this with MSVC, please?

Edit: forgot to derive from with_virtual_dtor, that's important!

[artemp]

@BergWerkGIS - could you try compiling the above code ^ ?

[wilhelmberg]

Those two alone build fine with VS2015:

• https://github.com/mapbox/variant
• https://github.com/lightmare/variant/tree/noexcept-destructor

going for a mapnik build with https://github.com/lightmare/variant/tree/noexcept-destructor next

[wilhelmberg]

Nope:

       "c:\mb\windows-builds-64\packages\mapnik-master\mapnik-gyp\build\mapnik.sln" (mapnik;csv;gdal;geojson;ogr;pgraster;postgis
       ;raster;shape;sqlite;topojson target) (1) ->
       "c:\mb\windows-builds-64\packages\mapnik-master\mapnik-gyp\build\gdal.vcxproj.metaproj" (default target) (7) ->
       "c:\mb\windows-builds-64\packages\mapnik-master\mapnik-gyp\build\gdal.vcxproj" (default target) (8) ->
       (ClCompile target) ->
         c:\mb\windows-builds-64\packages\mapnik-master\plugins\input\gdal\gdal_featureset.hpp(71): error C2694: 'gdal_featureset
       ::~gdal_featureset(void) noexcept(<expr>)': overriding virtual function has less restrictive exception specification than
       base class virtual member function 'mapnik::Featureset::~Featureset(void)' (compiling source file ..\..\plugins\input\gdal
       \gdal_datasource.cpp) [c:\mb\windows-builds-64\packages\mapnik-master\mapnik-gyp\build\gdal.vcxproj]
         c:\mb\windows-builds-64\packages\mapnik-master\plugins\input\gdal\gdal_featureset.hpp(71): error C2694: 'gdal_featureset
       ::~gdal_featureset(void) noexcept(<expr>)': overriding virtual function has less restrictive exception specification than
       base class virtual member function 'mapnik::Featureset::~Featureset(void)' (compiling source file ..\..\plugins\input\gdal
       \gdal_featureset.cpp) [c:\mb\windows-builds-64\packages\mapnik-master\mapnik-gyp\build\gdal.vcxproj]
         ..\..\plugins\input\gdal\gdal_featureset.cpp(79): fatal error C1903: unable to recover from previous error(s); stopping
       compilation [c:\mb\windows-builds-64\packages\mapnik-master\mapnik-gyp\build\gdal.vcxproj]
         cl : Command line error D8040: error creating or communicating with child process [c:\mb\windows-builds-64\packages\mapn
       ik-master\mapnik-gyp\build\gdal.vcxproj]

    818 Warning(s)
    4 Error(s)

[lightmare]

Found another noexcept on dtor: https://github.com/mapbox/variant/blob/master/recursive_wrapper.hpp#L46 Can you try without it:

 ~recursive_wrapper() /*noexcept*/ { delete p_; };

And if that compiles, try explicit:

 ~recursive_wrapper() noexcept(std::is_nothrow_destructible<T>::value) { delete p_; };

If both compile, then maybe commenting out https://github.com/lightmare/variant/commit/5b9528c3e8a68f94aa53e49ed5f54bec2a51634c isn't necessary.

[wilhelmberg]

~recursive_wrapper() /*noexcept*/ { delete p_; };

:white_check_mark:

~recursive_wrapper() noexcept(std::is_nothrow_destructible<T>::value) { delete p_; };

:x:

C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\vector(671): error C3672: pseudo-destructor expression can
       only be used as part of a function call (compiling source file ..\..\src\json\mapnik_json_feature_collection_grammar.cpp)
       [c:\mb\windows-builds-64\packages\mapnik-master\mapnik-gyp\build\mapnik-json.vcxproj]
     C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\vector(671): error C3672: pseudo-destructor expression can
       only be used as part of a function call (compiling source file ..\..\src\json\mapnik_json_feature_grammar.cpp) [c:\mb\wind
       ows-builds-64\packages\mapnik-master\mapnik-gyp\build\mapnik-json.vcxproj]

       "c:\mb\windows-builds-64\packages\mapnik-master\mapnik-gyp\build\mapnik.sln" (mapnik-json;mapnik-wkt target) (1) ->
       "c:\mb\windows-builds-64\packages\mapnik-master\mapnik-gyp\build\mapnik-json.vcxproj" (default target) (2) ->
       (ClCompile target) ->
         C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\xmemory0(583): error C3672: pseudo-destructor expression
       can only be used as part of a function call (compiling source file ..\..\src\json\mapnik_json_generator_grammar.cpp) [c:\m
       b\windows-builds-64\packages\mapnik-master\mapnik-gyp\build\mapnik-json.vcxproj]
         C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\vector(671): error C3672: pseudo-destructor expression ca
       n only be used as part of a function call (compiling source file ..\..\src\json\mapnik_json_generator_grammar.cpp) [c:\mb\
       windows-builds-64\packages\mapnik-master\mapnik-gyp\build\mapnik-json.vcxproj]
         C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\xmemory0(583): error C3672: pseudo-destructor expression
       can only be used as part of a function call (compiling source file ..\..\src\json\mapnik_topojson_grammar.cpp) [c:\mb\wind
       ows-builds-64\packages\mapnik-master\mapnik-gyp\build\mapnik-json.vcxproj]
         C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\vector(671): error C3672: pseudo-destructor expression ca
       n only be used as part of a function call (compiling source file ..\..\src\json\mapnik_topojson_grammar.cpp) [c:\mb\window
       s-builds-64\packages\mapnik-master\mapnik-gyp\build\mapnik-json.vcxproj]
         C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\xmemory0(583): error C3672: pseudo-destructor expression
       can only be used as part of a function call (compiling source file ..\..\src\json\mapnik_json_feature_collection_grammar.c
       pp) [c:\mb\windows-builds-64\packages\mapnik-master\mapnik-gyp\build\mapnik-json.vcxproj]
         C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\xmemory0(583): error C3672: pseudo-destructor expression
       can only be used as part of a function call (compiling source file ..\..\src\json\mapnik_json_feature_grammar.cpp) [c:\mb\
       windows-builds-64\packages\mapnik-master\mapnik-gyp\build\mapnik-json.vcxproj]
         C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\vector(671): error C3672: pseudo-destructor expression ca
       n only be used as part of a function call (compiling source file ..\..\src\json\mapnik_json_feature_collection_grammar.cpp
       ) [c:\mb\windows-builds-64\packages\mapnik-master\mapnik-gyp\build\mapnik-json.vcxproj]
         C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\vector(671): error C3672: pseudo-destructor expression ca
       n only be used as part of a function call (compiling source file ..\..\src\json\mapnik_json_feature_grammar.cpp) [c:\mb\wi
       ndows-builds-64\packages\mapnik-master\mapnik-gyp\build\mapnik-json.vcxproj]

    0 Warning(s)
    8 Error(s)

[lightmare]

@BergWerkGIS Thanks, I will update the pull with a possible workaround.

[lightmare]

Okay, the only quick solution is:

~reference_wrapper() noexcept(false)

because if we had:

~variant() noexcept(std::is_nothrow_destructible<std::tuple<Types...>>::value) {}
// and
~recursive_wrapper() noexcept(std::is_nothrow_destructible<T>::value) {}

Then there'd be infinite recursion -- variant's nothrow-destructibility depends on recursive_wrapper's, and that in turn depends on variant's.

[tomilov]

The mission of recursive_wrapper is to wrap incomplete type. One of the two: variant definition or recursive_wrapper's wrapped type definition, — will be first. Second of them will be just only forward declared at that moment.

[tomilov]

The only workaround thinkable is to forward provide ad-hoc type traits as part of the recursive_wrapper type. I.e. template< typename wrapped_type, bool is_default_constructible, bool is_nothrow_destructible /* etc... plenty of them */ > class recursive_wrapper.

[lightmare]

They both need to be complete at the point where destructor is instantiated. And I actually got a template instantiation depth limit reached error when I tried the above.

[lightmare]

The only workaround thinkable is to forward provide ad-hoc type traits as part of the recursive_wrapper type. I.e. template< typename wrapped_type, bool is_default_constructible, bool is_nothrow_destructible /* etc... plenty of them */ > class recursive_wrapper.

:+1: but I think for now I'll "hot-fix" that with "don't put stuff with throwing dtor in recursive_wrapper"

[tomilov]

@lightmare fair enough.

[springmeyer]

The noexcept specifier on the destructor creeped into the code in https://github.com/mapbox/variant/commit/46622cbc920f2911c6dd8a285574a4f5f8c66f5f. I presume the easiest solution is to revert back and remove it to dodge https://github.com/mapnik/mapnik/issues/3277#issuecomment-177471394.

[lightmare]

It was not this noexcept that caused trouble, it was the complicated one involving (broken) is_nothrow_destructible that I added later (and was reverted already). Without explicit noexcept it's computed from bases' and members' destructors (i.e. everything that happens after this destructor's body), which in this case is the same as noexcept or noexcept(true).

[springmeyer]

@lightmare okay, thanks for clarifying. So do I understand right then that https://github.com/mapnik/mapnik/issues/3277#issuecomment-177471394 is likely fixed / no longer relevant as long as mapnik is using the latest variant (which it is)?

[springmeyer]

So do I understand right then that mapnik/mapnik#3277 (comment) is likely fixed

Answered my own question. https://github.com/mapnik/mapnik/issues/3277#issuecomment-184896603. This issue is therefore resolved, closing!