Skip to content

build: re-enable deprecated warning copy #30236

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
fanquake merged 1 commit into from
Jun 6, 2024
Merged

build: re-enable deprecated warning copy #30236

fanquake merged 1 commit into from
Jun 6, 2024

Conversation

@theuni
Copy link
Member

@theuni theuni commented Jun 5, 2024

@theuni
build: re-enable deprecated warning copy
c3a5e8a 
This was disabled in bitcoin#18738 due to the combo of old gcc and qt, neither of
which are relevant to us anymore.
@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 5, 2024 •
edited
Loading

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK maflcko, fanquake

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #30235 (build: warn on self-assignment by theuni)

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.

@DrahtBot DrahtBot mentioned this pull request Jun 6, 2024
Merged
@maflcko
Copy link
Member

maflcko commented Jun 6, 2024

ACK c3a5e8a

fanquake
fanquake approved these changes Jun 6, 2024
View reviewed changes
Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

ACK c3a5e8a - this is in -Wextra for Clang and GCC.

@fanquake fanquake merged commit f47cd64 into bitcoin:master Jun 6, 2024
@maflcko
Copy link
Member

maflcko commented Jun 6, 2024 •
edited
Loading

An alternative/addition would be to have one CI task compile with C++23, but that can be done after the cmake transition (remind me)

@fanquake
Copy link
Member

fanquake commented Jun 6, 2024

An alternative/addition would be to have one CI task compile with C++23, but that can be done after the cmake transition (remind me)

Can you clarify how this PR is related to C++23? I don't see CMake as a blocker, if that's something we want to do.

hebasto
hebasto reviewed Jun 6, 2024
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

Post-merge ACK c3a5e8a.

@maflcko
Copy link
Member

maflcko commented Jun 6, 2024

An alternative/addition would be to have one CI task compile with C++23, but that can be done after the cmake transition (remind me)

Can you clarify how this PR is related to C++23? I don't see CMake as a blocker, if that's something we want to do.

Ah sorry, looks like they didn't change anything about the deprecation in C++23 and it is kept as-is for now.

However, I found that this patch seems incomplete. For example, no warning is printed to catch https://eel.is/c++draft/depr.impldec, unless -Wdeprecated-copy-dtor is passed. See https://godbolt.org/z/GTGbWGEnr

@maflcko
Copy link
Member

maflcko commented Jun 6, 2024

For reference https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p2139r2.html#3.8 said:

 Strong recommendation: take decisive action early in the C++23 development cycle, so early adopters can shake out the remaining cost of updating old code. Note that this would be both an API and ABI breaking change, and it would be good to set that precedent early if we wish to allow such breakage in C++23.

But that didn't happen.

@hebasto
Copy link
Member

hebasto commented Jun 6, 2024

CMake includes dependencies with -isystem, which is equivalent to suppress_external_warnings = yes. Therefore, the block removed in this PR has never been a part of the CMake staging branch.

@fanquake
Copy link
Member

fanquake commented Jun 6, 2024 •
edited
Loading

unless -Wdeprecated-copy-dtor is passed.

I think we can turn on additional warning flags, if you think that is valuable.

CMake includes dependencies with -isystem, which is equivalent to suppress_external_warnings = yes.

We may still need a way to disable that functionality, so all warnings can be observed, so I think this block (or atleast the functionality) is still relevant.

@fanquake
Copy link
Member

fanquake commented Jun 6, 2024

See comments starting here: #30189 (comment). cc @maflcko.

@maflcko
Copy link
Member

maflcko commented Jun 6, 2024

unless -Wdeprecated-copy-dtor is passed.

I think we can turn on additional warning flags, if you think that is valuable.

Yeah, not sure how useful that'd be, given that the deprecation apparently won't result in a deletion (for now).

CMake includes dependencies with -isystem, which is equivalent to suppress_external_warnings = yes.

We may still need a way to disable that functionality, so all warnings can be observed, so I think this block (or atleast the functionality) is still relevant.

Yeah, could open a tracking issue in the cmake repo (hebasto/bitcoin)?

@fanquake
Copy link
Member

fanquake commented Jun 6, 2024

Yeah, could open a tracking issue in the cmake repo (hebasto/bitcoin)?

Opened one here so it isn't forgotten: hebasto#223.

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request May 15, 2025
@fanquake @PastaPastaPasta
Merge bitcoin#30236: build: re-enable deprecated warning copy
325c8f0 
c3a5e8a build: re-enable deprecated warning copy (Cory Fields)

Pull request description:

  Noticed while looking at the `-wno-*` flags in bitcoin#30235.

  This was disabled in bitcoin#18738 due to the combo of old gcc and qt. We no longer support the affected gcc, and the old qt should no longer be relevant to us anyway.

  See old fixes in:
  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88136
  and
  https://bugreports.qt.io/browse/QTBUG-75210
  and
  https://codereview.qt-project.org/c/qt/qtbase/+/245434

ACKs for top commit:
  maflcko:
    ACK c3a5e8a
  fanquake:
    ACK c3a5e8a - this is in `-Wextra` for Clang and GCC.

Tree-SHA512: bd008dc50134d15ca3bb0c4f78d910db5b7a0ee98b04c159122a6f13a24b18827806492f053293d9cc1f1528ba60dea6d9ed31a366f63840ccb7c55f002d263b
PastaPastaPasta pushed a commit to DashCoreAutoGuix/dash that referenced this pull request May 16, 2025
@fanquake @PastaPastaPasta
Merge bitcoin#30236: build: re-enable deprecated warning copy
5eb3dca 
c3a5e8a build: re-enable deprecated warning copy (Cory Fields)

Pull request description:

  Noticed while looking at the `-wno-*` flags in bitcoin#30235.

  This was disabled in bitcoin#18738 due to the combo of old gcc and qt. We no longer support the affected gcc, and the old qt should no longer be relevant to us anyway.

  See old fixes in:
  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88136
  and
  https://bugreports.qt.io/browse/QTBUG-75210
  and
  https://codereview.qt-project.org/c/qt/qtbase/+/245434

ACKs for top commit:
  maflcko:
    ACK c3a5e8a
  fanquake:
    ACK c3a5e8a - this is in `-Wextra` for Clang and GCC.

Tree-SHA512: bd008dc50134d15ca3bb0c4f78d910db5b7a0ee98b04c159122a6f13a24b18827806492f053293d9cc1f1528ba60dea6d9ed31a366f63840ccb7c55f002d263b
PastaPastaPasta added a commit to dashpay/dash that referenced this pull request May 19, 2025
@PastaPastaPasta
Merge #6668: backport: trivial 2025 05 08
1cf6a67 
6dfb41e Merge bitcoin#30235: build: warn on self-assignment (merge-script)
325c8f0 Merge bitcoin#30236: build: re-enable deprecated warning copy (merge-script)
796e6ee Merge bitcoin#29882: netbase: clean up Proxy logging (merge-script)
9116563 Merge bitcoin#29315: refactor: Compile unreachable walletdb code (Ava Chow)
ddbe865 Merge bitcoin#28672: build: move `-fstack-reuse=none` to CORE_CXXFLAGS (fanquake)
5349c5e Merge bitcoin#27918: fuzz: addrman, avoid `ConsumeDeserializable` when possible (fanquake)
da71765 Merge bitcoin#28091: fuzz: use `ConnmanTestMsg` in `connman` (fanquake)
768f07c Merge bitcoin#27420: build: remove ancient unused define (fanquake)
84ce049 Merge bitcoin#25985: Revert "build: Use Homebrew's sqlite package if it is available" (Andrew Chow)
4d9b355 Merge bitcoin#26070: build: Quiet warnings in symlinked headers installed from homebrew (fanquake)
f8630ba Merge bitcoin#25906: test: add coverage for invalid parameters for `rescanblockchain` (MacroFake)
494b7c0 Merge bitcoin#25282: Bugfix: configure: Define default for use_libevent (laanwj)
ca63ea7 Merge bitcoin#24958: build: Fix macOS Apple M1 build with miniupnpc and libnatpmp. Again :) (laanwj)
075f8db Merge bitcoin#24696: ci: Use monterey-xcode-13.3 (MarcoFalke)

Pull request description:

  ## Issue being fixed or feature implemented
  Batch of trivial backports

  ## How Has This Been Tested?
  CI

  ## Breaking Changes

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK 6dfb41e
  knst:
    utACK 6dfb41e

Tree-SHA512: ff0eb987feadbefa9493abc2a727ce7df6249ae1d82e929c44a90a9b23eff95d6f4f7d07a85c5b9a8ff54cf98b1b4bfa490fc6b38b705eae3f1d890383a323e6
@bitcoin bitcoin locked and limited conversation to collaborators Jun 6, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@hebasto hebasto hebasto left review comments

@fanquake fanquake fanquake approved these changes

Assignees

No one assigned

Labels

Build system

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@theuni @DrahtBot @maflcko @fanquake @hebasto