Bitcoin Core Github
43 subscribers
123K links
Download Telegram
āœ… maflcko closed an issue: "RFC: Replacing `tinyformat` with `{fmt}`"
(https://github.com/bitcoin/bitcoin/issues/33942)
šŸ’¬ maflcko commented on issue "RFC: Replacing `tinyformat` with `{fmt}`":
(https://github.com/bitcoin/bitcoin/issues/33942#issuecomment-3575193582)
> In addition to all of that, I think that will be a good step toward migrating to C++20 in the future, and we will be able to depend on the standard format library itself.

C++20 is already used, so I'll go ahead and close this for now.


I don't think there is value in switching `tinyformat` to `{fmt}` and then again to `std::format`, with all the churn.

If the switch is done, it should be straight to `std::format`.

So I'll close this for now, but the discussion can continue.
šŸ’¬ hebasto commented on pull request "depends: Fix native_capnp to respect build_CC and build_CXX":
(https://github.com/bitcoin/bitcoin/pull/33937#issuecomment-3575194223)
> Fixes #33859

I’m not convinced it does.

On the master branch @ 0690514d4f72aac251ee0b876cded9187d42c63e:
```
$ gmake --no-print-directory -C depends print-native_capnp_cxx CXX=clang++
native_capnp_cxx=g++
```

With this PR, the behaviour is unchanged:
```
$ gmake --no-print-directory -C depends print-native_capnp_cxx CXX=clang++
native_capnp_cxx=g++
```
šŸ’¬ diegoviola commented on pull request "Revert "gui, qt: brintToFront workaround for Wayland"":
(https://github.com/bitcoin-core/gui/pull/914#issuecomment-3575197345)
> Instead of linking to a bunch of threads, it could make sense to just say that the issue has been fixed upstream in Qt6, so that the workaround is no longer needed?

The `Qt::WindowStaysOnTopHint` workaround was never going to work on Wayland. Clients don't get to dictate whether they stay on top or not. The compositor does, and you (the user) are supposed to control the compositor through configuration. This is a fundamental difference that a lot of people don't seem to understand.

It se
...
šŸ’¬ maflcko commented on issue "RFC: Replacing `tinyformat` with `{fmt}`":
(https://github.com/bitcoin/bitcoin/issues/33942#issuecomment-3575205474)
For further context: The current minimum required GCC version is 12.1 (https://github.com/bitcoin/bitcoin/blob/master/doc/dependencies.md#compiler), however `std::format` only comes with GCC libstdc++ 13.1
šŸ’¬ maflcko commented on pull request "depends: Fix native_capnp to respect build_CC and build_CXX":
(https://github.com/bitcoin/bitcoin/pull/33937#issuecomment-3575210686)
@psam21 Is this LLM generated? Have you tested this yourself?
šŸ’¬ hebasto commented on pull request "depends: Fix native_capnp to respect build_CC and build_CXX":
(https://github.com/bitcoin/bitcoin/pull/33937#issuecomment-3575216394)
The title says "depends: Fix native_capnp to respect build_CC and build_CXX", but it seems working as expected on the master:
```
$ gmake --no-print-directory -C depends print-native_capnp_cxx build_CXX=clang++
native_capnp_cxx=clang++
```
šŸ’¬ fanquake commented on pull request "contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#discussion_r2559669334)
Some of these are folders, but this will also work.
šŸ’¬ fanquake commented on pull request "contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#discussion_r2559669730)
Done.
šŸ’¬ hulxv commented on issue "RFC: Replacing `tinyformat` with `{fmt}`":
(https://github.com/bitcoin/bitcoin/issues/33942#issuecomment-3575220797)
> For further context: The current minimum required GCC version is 12.1 (https://github.com/bitcoin/bitcoin/blob/master/doc/dependencies.md#compiler), however `std::format` only comes with GCC libstdc++ 13.1

For that, I thought about switching first to `{fmt}` and after that, we can switch to `std::format` easily when we bump the minimum required GCC version
šŸ’¬ maflcko commented on issue "RFC: Replacing `tinyformat` with `{fmt}`":
(https://github.com/bitcoin/bitcoin/issues/33942#issuecomment-3575238806)
Yes, I understand, but there are no meaningful user-visible benefits:

* Performance does not matter, because formatting isn't done in hot loops
* A few KiB of binary size don't matter, because the binary is several MiB in size
šŸ’¬ fanquake commented on pull request "init: Split file path handling out of -asmap option":
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3575244180)
> I think having two options to control such a minor feature is unintuive and overkill. If that's what people like, no objection as it really isn't important. But my preference would be to put everything in the -asmap option:

I agree.
šŸš€ fanquake merged a pull request: "test: add `-alertnotify` test for large work invalid chain warning"
(https://github.com/bitcoin/bitcoin/pull/33893)
šŸ’¬ hulxv commented on issue "RFC: Replacing `tinyformat` with `{fmt}`":
(https://github.com/bitcoin/bitcoin/issues/33942#issuecomment-3575297752)
But from the developers' view, I think having features like compile-time error-checking and easy API and [other features](https://github.com/fmtlib/fmt?tab=readme-ov-file#features) will be helpful to have. At least it will make it a little bit easier. Maybe this is a little thing, but nice to have
šŸ’¬ maflcko commented on issue "RFC: Replacing `tinyformat` with `{fmt}`":
(https://github.com/bitcoin/bitcoin/issues/33942#issuecomment-3575329532)
There is compile-time error-checking already implemented for tinyformat in this codebase, so there is also no developer benefit of using `std::format` or `{fmt}`.
šŸ’¬ fanquake commented on pull request "txgraph: drop move assignment operator":
(https://github.com/bitcoin/bitcoin/pull/33862#issuecomment-3575373453)
Please rebase this.
šŸ‘ fanquake approved a pull request: "ci: Run GUI unit tests in cross-Windows task"
(https://github.com/bitcoin/bitcoin/pull/33919#pullrequestreview-3504833470)
ACK fa7ea497c3ef9366805e520205f2acf04d4d347b - didn't test.
šŸš€ fanquake merged a pull request: "ci: Run GUI unit tests in cross-Windows task"
(https://github.com/bitcoin/bitcoin/pull/33919)
šŸ‘ fanquake approved a pull request: "depends: Update Qt download link"
(https://github.com/bitcoin/bitcoin/pull/33918#pullrequestreview-3504837208)
ACK 50cbde3295b4b45d3ef2e5f787a33eea91ef38b5
āœ… fanquake closed an issue: "depends: fallback server missing Qt downloads"
(https://github.com/bitcoin/bitcoin/issues/33898)