trivial: remove boost::{function, lexical_cast} usage, backport bitcoin#25550, #24624, #24406, cleanup some gArgs usage in init #5986

kwvg · 2024-04-18T13:27:58Z

Additional Information

When building Dash with clang-16, I use the following CXXFLAGS:

-Werror -Werror=reorder -Werror=thread-safety -Wno-unused-command-line-argument -Wno-deprecated-builtins -Wno-deprecated-volatile -Wno-ambiguous-reversed-operator -Wno-deprecated-enum-enum-conversion

The explanations of the -Wno* flags are as follows:

Argument	Reason
`-Wno-unused-command-line-argument`	Lingering use of `-static-libstdc++`, a GCC-ism that isn't recognized as a valid argument in Clang (no longer an issue thanks to #5958, thanks knst!)
`-Wno-deprecated-builtins`	Due to usage of deprecated builtins in `nanobench` that exist even in the most current version (see below)
`-Wno-deprecated-volatile`	The detection code for C++20 in the CI image (based on Ubuntu `focal`) relies on deprecated volatiles (see below)
`-Wno-ambiguous-reversed-operator`	Due to an ambiguous operator in a Dash unit test (source) (see below)
`-Wno-deprecated-enum-enum-conversion`	Due to key combination code in Dash Qt (source) (see below)

-Wno-deprecated-builtins

> make
Making all in src
make[1]: Entering directory '/src/dash/src'
make[2]: Entering directory '/src/dash/src'
make[3]: Entering directory '/src/dash'
make[3]: Leaving directory '/src/dash'
  CXX      bench/bench_dash-addrman.o
In file included from bench/addrman.cpp:6:
In file included from ./bench/bench.h:16:
./bench/nanobench.h:952:13: error: builtin __has_trivial_copy is deprecated; use __is_trivially_copyable instead [-Werror,-Wdeprecated-builtins]
    return !ANKERL_NANOBENCH_IS_TRIVIALLY_COPYABLE(Decayed) || sizeof(Decayed) > sizeof(long) || std::is_pointer<Decayed>::value;
            ^
./bench/nanobench.h:107:57: note: expanded from macro 'ANKERL_NANOBENCH_IS_TRIVIALLY_COPYABLE'
#    define ANKERL_NANOBENCH_IS_TRIVIALLY_COPYABLE(...) __has_trivial_copy(__VA_ARGS__)
                                                        ^
1 error generated.
make[2]: *** [Makefile:14567: bench/bench_dash-addrman.o] Error 1
make[2]: Leaving directory '/src/dash/src'
make[1]: *** [Makefile:18568: all-recursive] Error 1
make[1]: Leaving directory '/src/dash/src'
make: *** [Makefile:800: all-recursive] Error 1

-Wno-deprecated-volatile

> ./configure [...]
[...]
checking whether clang++-16 supports C++20 features with -h std=c++20... no
checking whether clang++-16 supports C++20 features with -std=c++2a... no
checking whether clang++-16 supports C++20 features with +std=c++2a... no
checking whether clang++-16 supports C++20 features with -h std=c++2a... no
configure: error: *** A compiler with support for C++20 language features is required.

> cat config.log
This file contains any messages produced by compilers while
running configure, to aid debugging if configure makes a mistake.

It was created by Dash Core configure 21.0.0, which was
generated by GNU Autoconf 2.69.  Invocation command line was

  $ ./configure --prefix=/src/dash/depends/x86_64-pc-linux-gnu --enable-zmq --enable-reduce-exports --disable-crash-hooks --disable-stacktraces --enable-c++20 --enable-suppress-external-warnings --enable-debug --disable-ccache --with-sanitizers=thread
[...]
configure:4450: checking whether clang++-16 accepts -g
configure:4470: clang++-16 -c -g -I/src/dash/depends/x86_64-pc-linux-gnu/include/ -DDEBUG_LOCKORDER -DENABLE_DASH_DEBUG -DARENA_DEBUG conftest.cpp >&5
configure:4470: $? = 0
configure:4511: result: yes
configure:4537: checking whether make supports the include directive
configure:4552: make -f confmf.GNU && cat confinc.out
this is the am__doit target
configure:4555: $? = 0
configure:4574: result: yes (GNU style)
configure:4599: checking dependency style of clang++-16
configure:4710: result: gcc3
configure:4756: checking whether clang++-16 supports C++20 features with -std=c++20
configure:5558: clang++-16 -std=c++20 -c -pipe -O2 -Werror -Werror=reorder -Werror=thread-safety -Wno-deprecated-builtins -I/src/dash/depends/x86_64-pc-linux-gnu/include/ -DDEBUG_LOCKORDER -DENABLE_DASH_DEBUG -DARENA_DEBUG conftest.cpp >&5
conftest.cpp:109:36: error: volatile-qualified parameter type 'volatile int' is deprecated [-Werror,-Wdeprecated-volatile]
    test(const int c, volatile int v) // 'volatile is deprecated in C++20'
                                   ^
1 error generated.

-Wno-ambiguous-reversed-operator

> make
Making all in src
make[1]: Entering directory '/src/dash/src'
make[2]: Entering directory '/src/dash/src'
make[3]: Entering directory '/src/dash'
make[3]: Leaving directory '/src/dash'
[...]
  CXX      test/fuzz/fuzz-addrman.o
test/fuzz/addrman.cpp:329:22: error: ISO C++20 considers use of overloaded operator '==' (with operand types 'CAddrManDeterministic' and 'CAddrManDeterministic') to be ambiguous despite there being a unique best viable function [-Werror,-Wambiguous-reversed-operator]
    assert(addr_man1 == addr_man2);
           ~~~~~~~~~ ^  ~~~~~~~~~
/usr/include/assert.h:93:27: note: expanded from macro 'assert'
     (static_cast <bool> (expr)                                         \
                          ^~~~
test/fuzz/addrman.cpp:145:10: note: ambiguity is between a regular call to this operator and a call with the argument order reversed
    bool operator==(const CAddrManDeterministic& other)
         ^
test/fuzz/addrman.cpp:145:10: note: mark 'operator==' as const or add a matching 'operator!=' to resolve the ambiguity
1 error generated.
make[2]: *** [Makefile:15393: test/fuzz/fuzz-addrman.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[3]: Leaving directory '/src/dash/src/dashbls'
make[2]: Leaving directory '/src/dash/src'
make[1]: *** [Makefile:18568: all-recursive] Error 1
make[1]: Leaving directory '/src/dash/src'
make: *** [Makefile:800: all-recursive] Error 1

-Wno-deprecated-enum-enum-conversion

> make
Making all in src
make[1]: Entering directory '/src/dash/src'
make[2]: Entering directory '/src/dash/src'
make[3]: Entering directory '/src/dash'
make[3]: Leaving directory '/src/dash'
[...]
  CXX      qt/libbitcoinqt_a-bitcoingui.o
qt/bitcoingui.cpp:376:51: error: arithmetic between different enumeration types ('Qt::Modifier' and 'Qt::Key') is deprecated [-Werror,-Wdeprecated-enum-enum-conversion]
    quitAction->setShortcut(QKeySequence(Qt::CTRL + Qt::Key_Q));
                                         ~~~~~~~~ ^ ~~~~~~~~~
qt/bitcoingui.cpp:609:56: error: arithmetic between different enumeration types ('Qt::Modifier' and 'Qt::Key') is deprecated [-Werror,-Wdeprecated-enum-enum-conversion]
    minimize_action->setShortcut(QKeySequence(Qt::CTRL + Qt::Key_M));
                                              ~~~~~~~~ ^ ~~~~~~~~~
2 errors generated.
make[2]: *** [Makefile:12803: qt/libbitcoinqt_a-bitcoingui.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: Leaving directory '/src/dash/src'
make[1]: *** [Makefile:18568: all-recursive] Error 1
make[1]: Leaving directory '/src/dash/src'
make: *** [Makefile:800: all-recursive] Error 1

This pull request removes the need for the last two -Wno* flags, removes some leftover boost::{function, lexical_cast} usage and some gArgs usage where a local variable would be more applicable.

Additionally, in some builds, there is an deprecation warning (-Wdeprecation) for using [=] and relying on its implicit capture of this (see below), this can be seen during GCC builds but not Clang builds and correspond to a proposal to the C++20 standard (proposal).

It has also been mentioned at https://en.cppreference.com/w/cpp/language/lambda, "The implicit capture of *this when the capture default is = is deprecated. (since C++20)". This has also been remedied as part of this PR.

-Wdeprecated

> make -j10
[...]
  CXX      qt/libbitcoinqt_a-trafficgraphwidget.o
qt/optionsdialog.cpp: In lambda function:
qt/optionsdialog.cpp:195:63: warning: implicit capture of 'this' via '[=]' is deprecated in C++20 [-Wdeprecated]
  195 |     connect(appearance, &AppearanceWidget::appearanceChanged, [=](){
      |                                                               ^
qt/optionsdialog.cpp:195:63: note: add explicit 'this' or '*this' capture
qt/optionsdialog.cpp: In lambda function:
qt/optionsdialog.cpp:277:55: warning: implicit capture of 'this' via '[=]' is deprecated in C++20 [-Wdeprecated]
  277 |     connect(ui->coinJoinEnabled, &QCheckBox::clicked, [=](bool fChecked) {
      |                                                       ^
qt/optionsdialog.cpp:277:55: note: add explicit 'this' or '*this' capture
qt/optionsdialog.cpp: In lambda function:
qt/optionsdialog.cpp:293:45: warning: implicit capture of 'this' via '[=]' is deprecated in C++20 [-Wdeprecated]
  293 |     connect(this, &OptionsDialog::rejected, [=]() {
      |                                             ^
qt/optionsdialog.cpp:293:45: note: add explicit 'this' or '*this' capture
  CXX      qt/libbitcoinqt_a-utilitydialog.o
  CXX      qt/libbitcoinqt_a-addressbookpage.o
[...]

Checklist

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas (note: N/A)
I have added or updated relevant unit/integration/functional/e2e tests (note: N/A)
I have made corresponding changes to the documentation
I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

ci/test/00_setup_env_native_qt5.sh

knst

LGTM 3c1969a

Why is it draft?

src/qt/trafficgraphwidget.h

knst · 2024-04-19T10:00:49Z

-Werror=reorder

It is already enabled in our configure.ac: https://github.com/dashpay/dash/blob/master/configure.ac#L459

is it? Probably you don't need to specify that option anymore.

kwvg · 2024-04-19T15:03:52Z

is it? Probably you don't need to specify that option anymore

I remember needing to add it because I was working on one of the deglob PRs and everything looked good on my machine but CI highlighted an ordering issue. To make sure those errors get flagged locally, I added the flag manually just to make sure.

Why is it draft?

Because I need to confirm that the changes to the keyboard shortcuts haven't broken them.

EDIT: Have validated that keyboard shortcuts work as expected in an Ubuntu 22.04 VM.

kwvg · 2024-04-20T14:40:26Z

There seems to be some CI-specific issue based on how it's interacting with the feature_proxy unit test.

3c1969a is a known-good version of this PR (build), new changes were pushed with 94bb169 which caused failures with feature_proxy in all test instances (build). This seemed odd because this mirrors a similar failure (build) in #5987 (that, as of this writing is 54e35c0).

Considering the possibility that my changes in two PRs could have broken the same test, I spent a lot of time getting IPv6 to work properly locally only for feature_proxy to pass, despite multiple build configurations and branch resets. Since I cannot reproduce the problem locally, 3c1969a was pushed twice, once as-is and another time with a timestamp bump (3a2fe3a).

The latter being done because for a while, GitHub was re-using the same GitLab results instead of triggering a new build, though since then, it has triggered both builds for both the original and refreshed commit.

3c1969a and 3a2fe3a are the same commit (diff) and both 3c1969a (build) and 3a2fe3a (build), fail with CI.

Work on this PR has been put on halt for now.

EDIT: Worked around with #5988

PastaPastaPasta · 2024-04-23T14:39:48Z

range-diff looks good (build ci was happy previously as well)

 -:  ---------- >  1:  10a006e626 test: disable ipv6 tests for now
 -:  ---------- >  2:  6423f6bc5d Merge #21395: Net processing: Remove unused CNode.address member
 -:  ---------- >  3:  1a07e04a0e Merge #21370: Use C++11 member initializer in CNodeState
 -:  ---------- >  4:  666e6e2015 Merge #20406: util: Avoid invalid integer negation in FormatMoney and ValueFromAmount
 -:  ---------- >  5:  6ca0eb189e Merge #21007: bitcoind: Add -daemonwait option to wait for initialization
 -:  ---------- >  6:  99f09a0be6 Merge #20040: wallet: Refactor OutputGroups to handle fees and spending eligibility on grouping
 -:  ---------- >  7:  3325620ce1 Merge #21418: contrib: Make systemd invoke dependencies only when ready
 -:  ---------- >  8:  dd92322962 Merge #21447: Always add -daemonwait to known command line arguments
 -:  ---------- >  9:  b8aec5cb86 Merge #20197: p2p: protect onions in AttemptToEvictConnection(), add eviction protection test coverage
 -:  ---------- > 10:  daacafb539 Merge #21426: rpc: remove scantxoutset EXPERIMENTAL warning
 -:  ---------- > 11:  16ccb90bbb Merge bitcoin/bitcoin#21902: refactor: Remove useless extern keyword
 -:  ---------- > 12:  f3bc9535da Merge bitcoin/bitcoin#22187: test: Add sync_blocks in wallet_orphanedreward.py
 -:  ---------- > 13:  cc70886b4d Merge bitcoin/bitcoin#21795: fuzz: Terminate immediately if a fuzzing harness tries to perform a DNS lookup (belt and suspenders)
 -:  ---------- > 14:  b6dbd8bd7d Merge bitcoin/bitcoin#22092: test: convert documentation into type annotations
 -:  ---------- > 15:  2996daad4c (followup) bitcoin#19940
 -:  ---------- > 16:  348f4a8b9e Merge bitcoin/bitcoin#22121: doc: Various validation doc fixups
 -:  ---------- > 17:  26ff28a028 Merge bitcoin/bitcoin#21353: interfaces: Stop exposing wallet destdata to gui
 -:  ---------- > 18:  6c242da723 Merge bitcoin/bitcoin#21936: fuzz: Terminate immediately if a fuzzing harness tries to create a TCP socket (belt and suspenders)
 1:  33200e1285 = 19:  c47e9e78ed bench: drop leftover `boost::lexical_cast` benches, drop header from list
 2:  f07bd70f87 = 20:  9ae39f9ba7 qt: drop leftover `boost::function` usage in Qt, drop header from list
 3:  1bb2b46aa9 = 21:  fbc6bc88cf init: use local ArgsManager variable instead of global when possible
 4:  8236464359 = 22:  65585e68e6 merge bitcoin#25550: remove note on arm cross-compilation from build-unix.md
 5:  33ae52210f = 23:  3265b54a2f merge bitcoin#24624: Avoid potential -Wdeprecated-enum-enum-conversion warnings
 6:  3a2fe3ae58 = 24:  740d25c062 merge bitcoin#24406: Fix Wambiguous-reversed-operator compiler warnings

PastaPastaPasta

utACK 740d25c

knst

utACK 740d25c

…list `boost::lexical_cast` isn't used anywhere in Dash Core, the sole remaining use being in a benchmark, despite it no longer being used in Dash Core. Let's drop the benchmark and drop `boost/lexical_cast.hpp` from allowed Boost headers

We don't touch `CleanupBlockRevFiles` as the function is moved to blockstorage in bitcoin#21727, where it would need access to the global. GetBlocksDirPath() is non-const and invocations of that aren't included either.

…unix.md

…n warnings

…5986 c874205 fix(qt): tab switching via shortcuts doesn't work after 5986 (UdjinM6) Pull request description: ## Issue being fixed or feature implemented Looks like 3265b54 (#5986) broke it ## What was done? ## How Has This Been Tested? run dash-qt, try using shortcuts `cmd+1`, `cmd+2` etc. (on macos) ## Breaking Changes n/a ## Checklist: - [x] 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: PastaPastaPasta: ACK c874205 kwvg: ACK c874205 Tree-SHA512: 62a593ec804d75ff8aada0ef9ea90106adbf8cd11b202a6296086f55c2a4d2181e56dc8e56193a0ed49d94e55ee3236ab441ab477c8ca6d7b0c649dff987dbbc

kwvg added this to the 21 milestone Apr 18, 2024

knst reviewed Apr 18, 2024

View reviewed changes

ci/test/00_setup_env_native_qt5.sh Outdated Show resolved Hide resolved

kwvg force-pushed the trivial_fixes branch from bc817f1 to 3c1969a Compare April 18, 2024 13:54

kwvg requested a review from knst April 18, 2024 13:59

knst previously approved these changes Apr 19, 2024

View reviewed changes

src/qt/trafficgraphwidget.h Show resolved Hide resolved

kwvg dismissed knst’s stale review via 4d7f386 April 19, 2024 17:30

kwvg force-pushed the trivial_fixes branch 3 times, most recently from 3c1969a to 3a2fe3a Compare April 20, 2024 13:55

kwvg force-pushed the trivial_fixes branch from 3a2fe3a to 740d25c Compare April 23, 2024 14:35

kwvg marked this pull request as ready for review April 23, 2024 14:35

PastaPastaPasta approved these changes Apr 23, 2024

View reviewed changes

knst approved these changes Apr 23, 2024

View reviewed changes

PastaPastaPasta merged commit 5c240bb into dashpay:develop Apr 23, 2024
8 of 9 checks passed

kwvg added 6 commits April 23, 2024 15:34

qt: drop leftover boost::function usage in Qt, drop header from list

9ae39f9

merge bitcoin#25550: remove note on arm cross-compilation from build-…

65585e6

…unix.md

merge bitcoin#24624: Avoid potential -Wdeprecated-enum-enum-conversio…

3265b54

…n warnings

merge bitcoin#24406: Fix Wambiguous-reversed-operator compiler warnings

740d25c

UdjinM6 mentioned this pull request Apr 24, 2024

fix(qt): tab switching via shortcuts doesn't work after 5986 #5995

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

trivial: remove boost::{function, lexical_cast} usage, backport bitcoin#25550, #24624, #24406, cleanup some gArgs usage in init #5986

trivial: remove boost::{function, lexical_cast} usage, backport bitcoin#25550, #24624, #24406, cleanup some gArgs usage in init #5986

kwvg commented Apr 18, 2024 •

edited by knst

knst left a comment

knst commented Apr 19, 2024

kwvg commented Apr 19, 2024 •

edited

kwvg commented Apr 20, 2024 •

edited

PastaPastaPasta commented Apr 23, 2024 •

edited

PastaPastaPasta left a comment

knst left a comment

trivial: remove boost::{function, lexical_cast} usage, backport bitcoin#25550, #24624, #24406, cleanup some gArgs usage in init #5986

trivial: remove boost::{function, lexical_cast} usage, backport bitcoin#25550, #24624, #24406, cleanup some gArgs usage in init #5986

Conversation

kwvg commented Apr 18, 2024 • edited by knst

Additional Information

Checklist

knst left a comment

Choose a reason for hiding this comment

knst commented Apr 19, 2024

kwvg commented Apr 19, 2024 • edited

kwvg commented Apr 20, 2024 • edited

PastaPastaPasta commented Apr 23, 2024 • edited

PastaPastaPasta left a comment

Choose a reason for hiding this comment

knst left a comment

Choose a reason for hiding this comment

kwvg commented Apr 18, 2024 •

edited by knst

kwvg commented Apr 19, 2024 •

edited

kwvg commented Apr 20, 2024 •

edited

PastaPastaPasta commented Apr 23, 2024 •

edited