Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

jaykrell
Copy link

@jaykrell jaykrell commented Oct 21, 2021

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.:
boostorg/json#636

See also, similar from me:
boostorg/format#85

jaykrell added a commit to jaykrell/format that referenced this pull request Oct 21, 2021
/Zc:implicitNoexcept-.
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.:
    boostorg/json#636

  And proposed here:
    boostorg/iostreams#136

I grant there there could be more of this.
These two are just enough for our codebase.
@jaykrell jaykrell changed the title Fix to compile with newer Visual C++, such as 16.11, and /Zc:implicitNoexcept-. Fix to compile with Visual C++ and /Zc:implicitNoexcept-. Oct 21, 2021
@pdimov
Copy link
Member

pdimov commented Oct 21, 2021

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
Copy link
Author

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

@jaykrell
Copy link
Author

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

@mclow
Copy link
Contributor

mclow commented Oct 21, 2021

jaykrell added a commit to jaykrell/format that referenced this pull request Oct 21, 2021
@rdoeffinger
Copy link
Contributor

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
Copy link
Author

jaykrell commented Feb 2, 2022

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
Copy link
Contributor

mclow commented Feb 2, 2022

This looks OK to me.

…Noexcept-.

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.:
  boostorg/json#636
@jaykrell
Copy link
Author

@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.)

jaykrell added a commit to jaykrell/format that referenced this pull request Jul 12, 2024
/Zc:implicitNoexcept-.
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.:
    boostorg/json#636

  And proposed here:
    boostorg/iostreams#136

I grant there there could be more of this.
These two are just enough for our codebase.
@pdimov pdimov closed this Jul 13, 2024
@pdimov pdimov reopened this Jul 13, 2024
@pdimov
Copy link
Member

pdimov commented Jul 13, 2024

Closing/reopening to retrigger CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants