search

boostorg / iostreams

Boost.org iostreams module
http://boost.org/libs/iostreams
Boost Software License 1.0
43 stars 116 forks source link

Fix to compile with Visual C++ and /Zc:implicitNoexcept-. #136

Open jaykrell opened 2 years ago

jaykrell commented 2 years ago

Otherwise gets: Error C2694 'override': overriding virtual function has less restrictive exception specification than base class virtual member function 'base'

Similar changes are being made e.g.: https://github.com/boostorg/json/pull/636

See also, similar from me: https://github.com/boostorg/format/pull/85

pdimov commented 2 years ago

This PR doesn't look correct, because it uses noexcept and override unconditionally. This will cause errors for C++03 and on MSVC versions earlier than 2015 (or 2013, I don't remember which version introduced noexcept.)

These need to be BOOST_NOEXCEPT and BOOST_OVERRIDE, respectively. Perhaps just BOOST_NOEXCEPT, because override isn't necessary here.

jaykrell commented 2 years ago

Ok. Serious question then: instead of = default should I use { }? Or is that going to "too old" compiler?

jaykrell commented 2 years ago

Since you specifically said C++03 I suspect I should go with { }.

mclow commented 2 years ago

Since you specifically said C++03 I suspect I should go with { }.

How about BOOST_DEFAULTED_FUNCTION? (see https://www.boost.org/doc/libs/1_77_0/libs/config/doc/html/boost_config/boost_macro_reference.html#boost_config.boost_macro_reference.macros_that_allow_use_of_c__11_features_with_c__03_compilers)

rdoeffinger commented 2 years ago

I think you should squash the 3 commits into a single one, as it's not desirable to have not-quite-correct versions in the history. Note: I am not speaking with any particular authority, just making a drive-by comment...

jaykrell commented 2 years ago

I think you should squash the 3 commits into a single one, as it's not desirable to have not-quite-correct versions in the history. Note: I am not speaking with any particular authority, just making a drive-by comment...

I agree, but, is it approved othewise? That can be done upon commit in the GitHub gui, including editing the commit message.

Opinions vary and are strong about PR workflow. I've seen people, reviewing my code, request strongly never to rebase a PR, as it makes the PR harder to read. Even if then squash upon commit is the only choice.

mclow commented 2 years ago

This looks OK to me.

jaykrell commented 2 months ago

@mclow I squashed and rebased, ok? (Is squash really required? A lot of repositories are configured to do that upon commit, not optionally. Imho that is 99% the right policy. Unless maybe CI runs and passes at each commit, and users and reviewers agree the history reads ok/better with multiple commits. But I've never seen this.)

pdimov commented 2 months ago

Closing/reopening to retrigger CI.