Skip to content

Fix -Wmismatched-tags warnings #21051

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 2, 2021
Merged

Fix -Wmismatched-tags warnings #21051

merged 2 commits into from
Feb 2, 2021

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Feb 1, 2021 •
edited
Loading

Warnings were introduced in #20749:

./validation.h:43:1: warning: class 'CCheckpointData' was previously declared as a struct; this is valid, but may result in linker errors under the Microsoft C++ ABI [-Wmismatched-tags]
class CCheckpointData;
^
./chainparams.h:24:8: note: previous use is here
struct CCheckpointData {
       ^
./validation.h:43:1: note: did you mean struct here?
class CCheckpointData;
^~~~~
struct
1 warning generated.

This change fixes AppVeyor build: https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/37547435

@hebasto hebasto mentioned this pull request Feb 1, 2021
Merged
@jnewbery
Copy link
Contributor

jnewbery commented Feb 1, 2021

Code review ACK 1485124

Thanks @hebasto!

@practicalswift
Copy link
Contributor

cr ACK 1485124: patch looks correct

Thanks for fixing warnings!

@hebasto
Copy link
Member Author

hebasto commented Feb 1, 2021

Thanks for fixing warnings!

Not warnings only, but MSVC builds are fixed as well :)

@dongcarl
Copy link
Contributor

dongcarl commented Feb 1, 2021

Code Review ACK 1485124
🤦 @dongcarl

ryanofsky
ryanofsky approved these changes Feb 1, 2021
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK. But I unclear on how appveyor failed to catch this before merging. Or maybe changes were merged despite appveyor failing? I do see appveyor errors one of my recent pushes: https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/37552923

Would be nice to know what's going on with appveyor. Also ideally we would make either make this an error on linux, or make this not an error on windows to be consistent across platforms.

@ajtowns
Copy link
Contributor

ajtowns commented Feb 1, 2021

ACK 1485124

I think adding:

AX_CHECK_COMPILE_FLAG([-Werror=mismatched-tags],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=mismatched-tags"],,[[$CXXFLAG_WERROR]])

after the -Werror=unreachable-code-loop-increment line is all you need to make this an error for clang on linux when configuring with enable-werror.

@hebasto
Copy link
Member Author

hebasto commented Feb 1, 2021

Updated.

@glozow
Copy link
Member

glozow commented Feb 1, 2021

utACK b6aadcd 🚗

Thanks @hebasto, u so fast ⚡

@practicalswift
Copy link
Contributor

cr ACK b6aadcd: patch looks correct

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 1, 2021

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@fanquake fanquake merged commit f72d80b into bitcoin:master Feb 2, 2021
@DrahtBot DrahtBot mentioned this pull request Feb 2, 2021
Closed
This was referenced Feb 2, 2021
Merged
Merged
@jnewbery
Copy link
Contributor

jnewbery commented Feb 2, 2021

But I unclear on how appveyor failed to catch this before merging. Or maybe changes were merged despite appveyor failing?

This is at least partially my fault. I encouraged @laanwj to merge #20749. I'd seen a "QT download hash mismatch" error in appveyor and assumed it was spurious.

@hebasto hebasto deleted the 210201-warning branch February 2, 2021 08:47
@practicalswift practicalswift mentioned this pull request Feb 2, 2021
Merged
@laanwj
Copy link
Member

laanwj commented Feb 2, 2021

Appveyor is flaky enough, and has been flaky enough historically (with really strange issues like sudden compiler upgrades breaking compatibility) that I must admit to sometimes ignore it. Sorry for that in this case.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 3, 2021
@fanquake
Merge bitcoin#21051: Fix -Wmismatched-tags warnings
3a79b18 
b6aadcd build: Add -Werror=mismatched-tags (Hennadii Stepanov)
1485124 Fix -Wmismatched-tags warnings (Hennadii Stepanov)

Pull request description:

  Warnings were introduced in bitcoin#20749:
  ```
  ./validation.h:43:1: warning: class 'CCheckpointData' was previously declared as a struct; this is valid, but may result in linker errors under the Microsoft C++ ABI [-Wmismatched-tags]
  class CCheckpointData;
  ^
  ./chainparams.h:24:8: note: previous use is here
  struct CCheckpointData {
         ^
  ./validation.h:43:1: note: did you mean struct here?
  class CCheckpointData;
  ^~~~~
  struct
  1 warning generated.
  ```

  This change fixes AppVeyor build: https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/37547435

ACKs for top commit:
  glozow:
    utACK bitcoin@b6aadcd 🚗
  practicalswift:
    cr ACK b6aadcd: patch looks correct

Tree-SHA512: 3ac887ebdbf9a1ae33c1fd5381b3b8d83388ad557ddeb55013acd42bb9752a5bd009e3a0eed52644a023a7a0dda1c159277981af82f58fb0abfe60b84e01bf29
@ryanofsky ryanofsky mentioned this pull request Feb 3, 2021
Draft
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ryanofsky ryanofsky ryanofsky approved these changes

Assignees
No one assigned
Labels
Validation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@hebasto @jnewbery @practicalswift @dongcarl @ajtowns @glozow @DrahtBot @laanwj @ryanofsky @fanquake