💬 LarryRuane commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1120964573)
Nit, simpler. I don't know if the comment is necessary (the reader is expected to know this about maps), but it may help to document that we're taking advantage of that behavior.
The developer notes do [say](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures) not to use the `std::map []` syntax, but only for reading.
```suggestion
// This creates the map entry if it doesn't already exist.
m_requested_outpoints_by_txid[outpoint.ha
...
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1120964573)
Nit, simpler. I don't know if the comment is necessary (the reader is expected to know this about maps), but it may help to document that we're taking advantage of that behavior.
The developer notes do [say](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures) not to use the `std::map []` syntax, but only for reading.
```suggestion
// This creates the map entry if it doesn't already exist.
m_requested_outpoints_by_txid[outpoint.ha
...
💬 darosior commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1183410413)
This specific case is the very common match-all-variants-and-assert pattern so i think it's good like that. It's also documented this way in our style guidelines: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures.
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1183410413)
This specific case is the very common match-all-variants-and-assert pattern so i think it's good like that. It's also documented this way in our style guidelines: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures.
💬 MarcoFalke commented on pull request "refactor: Move chain constants to the util library":
(https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188847042)
Well, I mean that the `default` case can now be skipped. See https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures section `enum class` - `default:`
(https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188847042)
Well, I mean that the `default` case can now be skipped. See https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures section `enum class` - `default:`
💬 hebasto commented on pull request "refactor: Use ChainType enum exhaustively":
(https://github.com/bitcoin/bitcoin/pull/27611#discussion_r1189776962)
Our [Developer Notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures) also suggest a comment `// no default case, so the compiler can warn about missing cases`.
(https://github.com/bitcoin/bitcoin/pull/27611#discussion_r1189776962)
Our [Developer Notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures) also suggest a comment `// no default case, so the compiler can warn about missing cases`.
💬 MarcoFalke commented on issue "Intermittent failures in interface_usdt_mempool.py":
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1621292840)
Or do the `TRACEx` macros create a copy of the data in-line? If yes, it could make sense to document that in doc/tracing.md#c_str
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1621292840)
Or do the `TRACEx` macros create a copy of the data in-line? If yes, it could make sense to document that in doc/tracing.md#c_str
💬 fanquake commented on pull request "build: Require C++20 compiler":
(https://github.com/bitcoin/bitcoin/pull/28349#issuecomment-1750575332)
> Looks like CI fails with
My assumption is that a newer version of the macOS SDK is going to be needed, probably from Xcode 14.3 or 15, as that is where most c++20 support [has landed in Apples libc++](https://developer.apple.com/xcode/cpp/#c++20) (We currently use the version from Xcode 12). Note that building this branch, using Xcode 15, targetting `-mmacosx-version-min=11.0` work ok.
(https://github.com/bitcoin/bitcoin/pull/28349#issuecomment-1750575332)
> Looks like CI fails with
My assumption is that a newer version of the macOS SDK is going to be needed, probably from Xcode 14.3 or 15, as that is where most c++20 support [has landed in Apples libc++](https://developer.apple.com/xcode/cpp/#c++20) (We currently use the version from Xcode 12). Note that building this branch, using Xcode 15, targetting `-mmacosx-version-min=11.0` work ok.
💬 fanquake commented on pull request "[WIP] C++20 std::views::reverse":
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-1855524913)
> Interesting that the guix macOS build passed, given that it is on clang-15, no?
I'd guess this is because our macOS SDK (libc++) is from Xcode 15, which is based on LLVM ~16, and [includes support for ranges](https://developer.apple.com/xcode/cpp/#c++20). So as long as clang-15 can compile it, which it should given vanilla LLVM supports the use of ranges (experimentally) in 15, it probably works.
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-1855524913)
> Interesting that the guix macOS build passed, given that it is on clang-15, no?
I'd guess this is because our macOS SDK (libc++) is from Xcode 15, which is based on LLVM ~16, and [includes support for ranges](https://developer.apple.com/xcode/cpp/#c++20). So as long as clang-15 can compile it, which it should given vanilla LLVM supports the use of ranges (experimentally) in 15, it probably works.
💬 fanquake commented on pull request "[WIP] C++20 std::views::reverse":
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-1864200442)
> Though, on GHA it says XCode 14.3
According to https://developer.apple.com/xcode/cpp/#c++20, "Ranges" have been available on macOS since Xcode 14.3.
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-1864200442)
> Though, on GHA it says XCode 14.3
According to https://developer.apple.com/xcode/cpp/#c++20, "Ranges" have been available on macOS since Xcode 14.3.
💬 stickies-v commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1439476415)
nit
```suggestion
- **client class (in generated code)**: A C++ class generated from a Cap’n Proto interface which inherits from a Bitcoin Core abstract class, and implements each virtual method to send IPC requests to another process. (see also [components section](#c-client-subclasses-in-generated-code))
```
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1439476415)
nit
```suggestion
- **client class (in generated code)**: A C++ class generated from a Cap’n Proto interface which inherits from a Bitcoin Core abstract class, and implements each virtual method to send IPC requests to another process. (see also [components section](#c-client-subclasses-in-generated-code))
```
💬 fanquake commented on issue "Discussion: Upgrading to C++20":
(https://github.com/bitcoin/bitcoin/issues/23363#issuecomment-1914540362)
On the `std::format` front, looks like as of the next Xcode release (15.3), Apple is [adding support for `std::format`](https://developer.apple.com/documentation/xcode-release-notes/xcode-15_3-release-notes#C++-Standard-Library):
> The following new features have been implemented:
> P0645 - Text formatting (std::format)
(https://github.com/bitcoin/bitcoin/issues/23363#issuecomment-1914540362)
On the `std::format` front, looks like as of the next Xcode release (15.3), Apple is [adding support for `std::format`](https://developer.apple.com/documentation/xcode-release-notes/xcode-15_3-release-notes#C++-Standard-Library):
> The following new features have been implemented:
> P0645 - Text formatting (std::format)
💬 vasild commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1489725106)
Doxygen attaches this comment to the `TaskRunnerInterface` class in the generated documentation which I guess is not the intention.
```suggestion
/** @file
*
* This header provides an interface and simple implementation for a task
* runner. Another threaded, serial implementation using a queue is available in
* the scheduler module's SerialTaskRunner.
*/
/**
* Interface for a task runner.
*/
class TaskRunnerInterface
```
See https://www.doxygen.nl/manual/commands.html#c
...
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1489725106)
Doxygen attaches this comment to the `TaskRunnerInterface` class in the generated documentation which I guess is not the intention.
```suggestion
/** @file
*
* This header provides an interface and simple implementation for a task
* runner. Another threaded, serial implementation using a queue is available in
* the scheduler module's SerialTaskRunner.
*/
/**
* Interface for a task runner.
*/
class TaskRunnerInterface
```
See https://www.doxygen.nl/manual/commands.html#c
...
📝 fanquake opened a pull request: "build: add `-W[leading|trailing]-whitespace`"
(https://github.com/bitcoin/bitcoin/pull/32482)
GCC 15 now has options to turn these into compile failures: https://gcc.gnu.org/gcc-15/changes.html#c-family. Fix the few cases of leading tabs, and trailing whitespace, and then enable these options. Unfortunately, CMake/Qt will generate code that contains trailing whitespace:
```bash
[ 98%] Building CXX object src/qt/CMakeFiles/bitcoinqt.dir/bitcoinqt_autogen/EJRQKI7XPS/qrc_bitcoin_locale.cpp.o
/root/bitcoin/build/src/qt/bitcoinqt_autogen/EWIEGA46WW/qrc_bitcoin.cpp:5603:2: warning: trailing
...
(https://github.com/bitcoin/bitcoin/pull/32482)
GCC 15 now has options to turn these into compile failures: https://gcc.gnu.org/gcc-15/changes.html#c-family. Fix the few cases of leading tabs, and trailing whitespace, and then enable these options. Unfortunately, CMake/Qt will generate code that contains trailing whitespace:
```bash
[ 98%] Building CXX object src/qt/CMakeFiles/bitcoinqt.dir/bitcoinqt_autogen/EJRQKI7XPS/qrc_bitcoin_locale.cpp.o
/root/bitcoin/build/src/qt/bitcoinqt_autogen/EWIEGA46WW/qrc_bitcoin.cpp:5603:2: warning: trailing
...
💬 hebasto commented on pull request "build: add `-W[leading|trailing]-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-2876950405)
> GCC 15 now has options to turn these into compile failures: https://gcc.gnu.org/gcc-15/changes.html#c-family. Fix the few cases of leading tabs, and trailing whitespace, and then enable these options.
Concept ACK on that.
> Unfortunately, CMake/Qt will generate code that contains trailing whitespace:
>
> ```shell
> [ 98%] Building CXX object src/qt/CMakeFiles/bitcoinqt.dir/bitcoinqt_autogen/EJRQKI7XPS/qrc_bitcoin_locale.cpp.o
> /root/bitcoin/build/src/qt/bitcoinqt_autogen/EWIEGA46WW
...
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-2876950405)
> GCC 15 now has options to turn these into compile failures: https://gcc.gnu.org/gcc-15/changes.html#c-family. Fix the few cases of leading tabs, and trailing whitespace, and then enable these options.
Concept ACK on that.
> Unfortunately, CMake/Qt will generate code that contains trailing whitespace:
>
> ```shell
> [ 98%] Building CXX object src/qt/CMakeFiles/bitcoinqt.dir/bitcoinqt_autogen/EJRQKI7XPS/qrc_bitcoin_locale.cpp.o
> /root/bitcoin/build/src/qt/bitcoinqt_autogen/EWIEGA46WW
...
💬 hebasto commented on pull request "build: add `-W[leading|trailing]-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-2922854588)
> > GCC 15 now has options to turn these into compile failures: https://gcc.gnu.org/gcc-15/changes.html#c-family. Fix the few cases of leading tabs, and trailing whitespace, and then enable these options.
>
> Concept ACK on that.
>
> > Unfortunately, CMake/Qt will generate code that contains trailing whitespace:
> > ```shell
> > [ 98%] Building CXX object src/qt/CMakeFiles/bitcoinqt.dir/bitcoinqt_autogen/EJRQKI7XPS/qrc_bitcoin_locale.cpp.o
> > /root/bitcoin/build/src/qt/bitcoinqt_autoge
...
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-2922854588)
> > GCC 15 now has options to turn these into compile failures: https://gcc.gnu.org/gcc-15/changes.html#c-family. Fix the few cases of leading tabs, and trailing whitespace, and then enable these options.
>
> Concept ACK on that.
>
> > Unfortunately, CMake/Qt will generate code that contains trailing whitespace:
> > ```shell
> > [ 98%] Building CXX object src/qt/CMakeFiles/bitcoinqt.dir/bitcoinqt_autogen/EJRQKI7XPS/qrc_bitcoin_locale.cpp.o
> > /root/bitcoin/build/src/qt/bitcoinqt_autoge
...
💬 hebasto commented on pull request "build: add `-Wleading-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3168271627)
> GCC 15 now has options to turn leading & trailing whitespace into compile failures: https://gcc.gnu.org/gcc-15/changes.html#c-family. Fix the few cases of leading tabs, and trailing whitespace, and then enable `-Wleading-whitespace`. Unfortunately, CMake/Qt will generate code that contains trailing whitespace:
>
> ```shell
> [ 98%] Building CXX object src/qt/CMakeFiles/bitcoinqt.dir/bitcoinqt_autogen/EJRQKI7XPS/qrc_bitcoin_locale.cpp.o
> /root/bitcoin/build/src/qt/bitcoinqt_autogen/EWIEGA
...
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3168271627)
> GCC 15 now has options to turn leading & trailing whitespace into compile failures: https://gcc.gnu.org/gcc-15/changes.html#c-family. Fix the few cases of leading tabs, and trailing whitespace, and then enable `-Wleading-whitespace`. Unfortunately, CMake/Qt will generate code that contains trailing whitespace:
>
> ```shell
> [ 98%] Building CXX object src/qt/CMakeFiles/bitcoinqt.dir/bitcoinqt_autogen/EJRQKI7XPS/qrc_bitcoin_locale.cpp.o
> /root/bitcoin/build/src/qt/bitcoinqt_autogen/EWIEGA
...
💬 maflcko commented on pull request "ci: add libcpp hardening flags to macOS fuzz job":
(https://github.com/bitcoin/bitcoin/pull/33462#discussion_r2372747277)
hardening was added in xcode 16: https://developer.apple.com/documentation/xcode-release-notes/xcode-16-release-notes#C++-Standard-Library
```diff
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 52d21ef3ab..a0902e2700 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -145,8 +145,8 @@ jobs:
# Use the earliest Xcode supported by the version of macOS denoted in
# doc/release-notes-empty-template.md and providing at least t
...
(https://github.com/bitcoin/bitcoin/pull/33462#discussion_r2372747277)
hardening was added in xcode 16: https://developer.apple.com/documentation/xcode-release-notes/xcode-16-release-notes#C++-Standard-Library
```diff
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 52d21ef3ab..a0902e2700 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -145,8 +145,8 @@ jobs:
# Use the earliest Xcode supported by the version of macOS denoted in
# doc/release-notes-empty-template.md and providing at least t
...
💬 sedited commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2582578921)
Nit: It would be good to assert that we don't fall through here, as per the [developer-notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures), by adding an `assert(false)` here.
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2582578921)
Nit: It would be good to assert that we don't fall through here, as per the [developer-notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures), by adding an `assert(false)` here.
💬 sedited commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2592204362)
See the dev notes: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2592204362)
See the dev notes: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures