👍 l0rinc approved a pull request: "refactor: Use std::span over Span"
(https://github.com/bitcoin/bitcoin/pull/31519#pullrequestreview-2595295815)
I have recreated the changes locally and compared it against your change and reviewed the diffs one-by-one.
Based on my understanding, your changes are accurate, nicely transitioning to the new type in careful commits, you have thought of every corner case.
Rebased the change against latest master, all tests are passing locally as well and I'm getting the same scripted diff results (rewritten slightly for mac).
<details>
<summary>Span->std::span</summary>
```bash
gsed -i "s#\<Span\>#
...
(https://github.com/bitcoin/bitcoin/pull/31519#pullrequestreview-2595295815)
I have recreated the changes locally and compared it against your change and reviewed the diffs one-by-one.
Based on my understanding, your changes are accurate, nicely transitioning to the new type in careful commits, you have thought of every corner case.
Rebased the change against latest master, all tests are passing locally as well and I'm getting the same scripted diff results (rewritten slightly for mac).
<details>
<summary>Span->std::span</summary>
```bash
gsed -i "s#\<Span\>#
...
💬 l0rinc commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1942699465)
nit: for consistency:
```suggestion
/** Fill a byte span with random bytes. This overrides the RandomMixin version. */
```
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1942699465)
nit: for consistency:
```suggestion
/** Fill a byte span with random bytes. This overrides the RandomMixin version. */
```
💬 l0rinc commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1942642531)
Is this still accurate, now that we're always explicitly `const`-ing?
```
... but for const unsigned char member types
```
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1942642531)
Is this still accurate, now that we're always explicitly `const`-ing?
```
... but for const unsigned char member types
```
💬 l0rinc commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1942719114)
Reviewed it in more detail, `git diff --name-only HEAD~6..HEAD` basically only contains `doc/developer-notes.md` in addition to the previous commit - which doesn't contain a copyright header. The other skipped files don't contain any dates, so it's a correct change, nicely done.
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1942719114)
Reviewed it in more detail, `git diff --name-only HEAD~6..HEAD` basically only contains `doc/developer-notes.md` in addition to the previous commit - which doesn't contain a copyright header. The other skipped files don't contain any dates, so it's a correct change, nicely done.
💬 l0rinc commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1942700947)
nit:
```suggestion
template <typename T, size_t N> constexpr auto UCharSpanCast(std::span<T, N> s) -> std::span<std::remove_pointer_t<decltype(UCharCast(s.data()))>> { return {UCharCast(s.data()), s.size()}; }
```
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1942700947)
nit:
```suggestion
template <typename T, size_t N> constexpr auto UCharSpanCast(std::span<T, N> s) -> std::span<std::remove_pointer_t<decltype(UCharCast(s.data()))>> { return {UCharCast(s.data()), s.size()}; }
```
💬 hebasto commented on issue " Cannot figure out how to use std::atomic error for MacOS Sequoia 15.2":
(https://github.com/bitcoin/bitcoin/issues/31579#issuecomment-2636564824)
@egruper
> I can reproduce, attaching also build/CMakeFiles/CMakeConfigureLog.yaml.
Thank you for details provided!
The error message in the log file states: `'atomic' file not found`.
On your system, with the command-line tools installed, the `atomic` header should be located in the `/Library/Developer/CommandLineTools/usr/include/c++/v1` directory. If it is not, please try reinstalling the command-line tools.
(https://github.com/bitcoin/bitcoin/issues/31579#issuecomment-2636564824)
@egruper
> I can reproduce, attaching also build/CMakeFiles/CMakeConfigureLog.yaml.
Thank you for details provided!
The error message in the log file states: `'atomic' file not found`.
On your system, with the command-line tools installed, the `atomic` header should be located in the `/Library/Developer/CommandLineTools/usr/include/c++/v1` directory. If it is not, please try reinstalling the command-line tools.
💬 maflcko commented on pull request "func test: Expand tx download preference tests":
(https://github.com/bitcoin/bitcoin/pull/31437#issuecomment-2636597881)
ACK 846a1387280fa584f70ccb1f5d0198339b065528 🍕
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 846a1387280fa584f70ccb1f5d
...
(https://github.com/bitcoin/bitcoin/pull/31437#issuecomment-2636597881)
ACK 846a1387280fa584f70ccb1f5d0198339b065528 🍕
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 846a1387280fa584f70ccb1f5d
...
💬 vasild commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#issuecomment-2636601335)
@maflcko yes, I agree. Future changes that make message processing multi-threaded will have to take this into account.
(https://github.com/bitcoin/bitcoin/pull/31672#issuecomment-2636601335)
@maflcko yes, I agree. Future changes that make message processing multi-threaded will have to take this into account.
👍 vasild approved a pull request: "tracing: network connection tracepoints"
(https://github.com/bitcoin/bitcoin/pull/25832#pullrequestreview-2595610931)
ACK e3622a969293feea75cfadc8f7c6083edcd6d8de
(https://github.com/bitcoin/bitcoin/pull/25832#pullrequestreview-2595610931)
ACK e3622a969293feea75cfadc8f7c6083edcd6d8de
💬 maflcko commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1942840067)
The comment is for the second function below, see https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941880894
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1942840067)
The comment is for the second function below, see https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941880894
💬 egruper commented on issue " Cannot figure out how to use std::atomic error for MacOS Sequoia 15.2":
(https://github.com/bitcoin/bitcoin/issues/31579#issuecomment-2636659371)
@hebasto
Nice catch.
I indeed didn't have atomic under /Library/Developer/CommandLineTools/usr/include/c++/v1
I went on to 'rm -rf /Library/Developer/CommandLineTools' and re-install it with 'xcode-select --install' (didn't see your update so I didn't search under SDKs...)
Now I find atomic under:
/Library/Developer/CommandLineTools/SDKs/MacOSX15.2.sdk/usr/include/c++/v1/atomic
/Library/Developer/CommandLineTools/SDKs/MacOSX13.3.sdk/usr/include/c++/v1/atomic
/Library/Developer/CommandLineToo
...
(https://github.com/bitcoin/bitcoin/issues/31579#issuecomment-2636659371)
@hebasto
Nice catch.
I indeed didn't have atomic under /Library/Developer/CommandLineTools/usr/include/c++/v1
I went on to 'rm -rf /Library/Developer/CommandLineTools' and re-install it with 'xcode-select --install' (didn't see your update so I didn't search under SDKs...)
Now I find atomic under:
/Library/Developer/CommandLineTools/SDKs/MacOSX15.2.sdk/usr/include/c++/v1/atomic
/Library/Developer/CommandLineTools/SDKs/MacOSX13.3.sdk/usr/include/c++/v1/atomic
/Library/Developer/CommandLineToo
...
💬 maflcko commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1942852606)
Adjusted `span's`, so resolving this for now.
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1942852606)
Adjusted `span's`, so resolving this for now.
💬 hebasto commented on issue " Cannot figure out how to use std::atomic error for MacOS Sequoia 15.2":
(https://github.com/bitcoin/bitcoin/issues/31579#issuecomment-2636689705)
> And building with 'cmake -B build' works fine.
>
> Is this going to be logged somewhere?
In the configuration output, you can see:
```
-- Performing Test STD_ATOMIC_LINKS_WITHOUT_LIBATOMIC
-- Performing Test STD_ATOMIC_LINKS_WITHOUT_LIBATOMIC - Success
```
Also you could grep for `ATOMIC` in the `CMakeFiles/CMakeConfigureLog.yaml` log file.
(https://github.com/bitcoin/bitcoin/issues/31579#issuecomment-2636689705)
> And building with 'cmake -B build' works fine.
>
> Is this going to be logged somewhere?
In the configuration output, you can see:
```
-- Performing Test STD_ATOMIC_LINKS_WITHOUT_LIBATOMIC
-- Performing Test STD_ATOMIC_LINKS_WITHOUT_LIBATOMIC - Success
```
Also you could grep for `ATOMIC` in the `CMakeFiles/CMakeConfigureLog.yaml` log file.
💬 hebasto commented on issue " Cannot figure out how to use std::atomic error for MacOS Sequoia 15.2":
(https://github.com/bitcoin/bitcoin/issues/31579#issuecomment-2636702426)
I assume this issue can now be closed.
(https://github.com/bitcoin/bitcoin/issues/31579#issuecomment-2636702426)
I assume this issue can now be closed.
✅ maflcko closed an issue: " Cannot figure out how to use std::atomic error for MacOS Sequoia 15.2"
(https://github.com/bitcoin/bitcoin/issues/31579)
(https://github.com/bitcoin/bitcoin/issues/31579)
💬 maflcko commented on issue " Cannot figure out how to use std::atomic error for MacOS Sequoia 15.2":
(https://github.com/bitcoin/bitcoin/issues/31579#issuecomment-2636783895)
Closing for now, as this seemed to be an upstream issue that was fixed by a re-install.
(https://github.com/bitcoin/bitcoin/issues/31579#issuecomment-2636783895)
Closing for now, as this seemed to be an upstream issue that was fixed by a re-install.
💬 maflcko commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1942903118)
thx, done
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1942903118)
thx, done
💬 maflcko commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1942903310)
thx, done
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1942903310)
thx, done
💬 maflcko commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1942903540)
Pushed the typo fix, so closing this for now.
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1942903540)
Pushed the typo fix, so closing this for now.
💬 l0rinc commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#issuecomment-2636796621)
ACK fa77685ba5c7185781acca04f57399cdcd19e9f7
(https://github.com/bitcoin/bitcoin/pull/31519#issuecomment-2636796621)
ACK fa77685ba5c7185781acca04f57399cdcd19e9f7