Skip to content

Fix -Wmismatched-tags warnings#21051

Merged
fanquake merged 2 commits into
bitcoin:masterfrom
hebasto:210201-warning
Feb 2, 2021
Merged

Fix -Wmismatched-tags warnings#21051
fanquake merged 2 commits into
bitcoin:masterfrom
hebasto:210201-warning

Conversation

@hebasto

@hebasto hebasto commented Feb 1, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

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

jnewbery commented Feb 1, 2021

Copy link
Copy Markdown
Contributor

Code review ACK 1485124

Thanks @hebasto!

@practicalswift

practicalswift commented Feb 1, 2021

Copy link
Copy Markdown
Contributor

cr ACK 1485124: patch looks correct

Thanks for fixing warnings!

@hebasto

hebasto commented Feb 1, 2021

Copy link
Copy Markdown
Member Author

Thanks for fixing warnings!

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

@dongcarl

dongcarl commented Feb 1, 2021

Copy link
Copy Markdown
Contributor

Code Review ACK 1485124
🤦 @dongcarl

ryanofsky
ryanofsky approved these changes Feb 1, 2021
View reviewed changes

@ryanofsky ryanofsky left a comment

Copy link
Copy Markdown
Contributor

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

ajtowns commented Feb 1, 2021

Copy link
Copy Markdown
Contributor

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

hebasto commented Feb 1, 2021

Copy link
Copy Markdown
Member Author

Updated.

@glozow

glozow commented Feb 1, 2021

Copy link
Copy Markdown
Member

utACK b6aadcd 🚗

Thanks @hebasto, u so fast ⚡

@practicalswift

practicalswift commented Feb 1, 2021

Copy link
Copy Markdown
Contributor

cr ACK b6aadcd: patch looks correct

@DrahtBot

DrahtBot commented Feb 1, 2021

Copy link
Copy Markdown
Contributor

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

jnewbery commented Feb 2, 2021

Copy link
Copy Markdown
Contributor

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

laanwj commented Feb 2, 2021

Copy link
Copy Markdown
Member

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