💬 maflcko commented on pull request "refactor: Drop unused `#include <boost/operators.hpp>`":
(https://github.com/bitcoin/bitcoin/pull/32668#issuecomment-3027737893)
>I think any changes should be a part of #31308.

I'd say it is also fine to do it in smaller steps, but no strong opinion.

💬 hodlinator commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r2168558603)
```shell
₿ mypy contrib/devtools/reg-settings.py
contrib/devtools/reg-settings.py:99: SyntaxWarning: invalid escape sequence '\|'
"git", "grep", "-l", "AddArg(\|AddHiddenArgs(\|GetArg(\|GetPathArg(\|GetIntArg(\|GetBoolArg(\|GetArgs(\|IsArgSet(\|IsArgNegated(", "--", src_dir
```
Suggestion; make it an r-string:
```python
"git", "grep", "-l", r"AddArg(\|AddHiddenArgs(\|GetArg(\|GetPathArg(\|GetIntArg(\|GetBoolArg(\|GetArgs(\|IsArgSet(\|IsArgNegated(", "--", src_dir
```

💬 hodlinator commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r2168428599)
1. You described it in the middle of https://github.com/bitcoin/bitcoin/pull/31260#pullrequestreview-2455631901 but please document what `legacy` is intended to signify in this context using a comment or at minimum the commit message.

2. The field is currently never read. Should make that clear too in comments/commit message. Or remove it altogether for now, or add the functionality that reads from it into this PR.

🤔 hodlinator reviewed a pull request: "scripted-diff: Type-safe settings retrieval"
(https://github.com/bitcoin/bitcoin/pull/31260#pullrequestreview-2960951696)
Overdue Concept ACK b3968352b292c8c69dffbaa9c54a17405e246289

Looking back at this, I realize there's actually not *that* much going on. No (intended) changes in behavior when executing the code, just installing compile time guardrails.

💬 hodlinator commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r2168478592)
Commit message mentions *lint-format-strings.py* but commit doesn't touch it (any longer?).

💬 hodlinator commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r2175982324)
nit: Tempting to go for:
```suggestion
consteval int SettingFlags(SettingOptions options)
```
But it's somewhat of a departure from the surrounding style.

💬 hodlinator commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r2178368647)
nit: This indentation of the `for`-loops is not necessary, we can still access `content` after letting go of `f`. Less indentation => less intimidation. Same is already done in `replace_in_file()`.

💬 hodlinator commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r2178376601)
nit: Would be good to follow with `assert data_type` to guard against the `None`-case.

Similar case:
https://github.com/bitcoin/bitcoin/blob/73a77f4f01fc479bf015605cb95894b440533a2d/contrib/devtools/reg-settings.py#L400-L407

💬 hodlinator commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r2168340344)
nit:
```suggestion
//! Setting template struct used to declare name, type, and behavior of command
```

💬 hodlinator commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r2168630944)
Leftover line of code?

💬 hodlinator commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r2178387847)
nit: `self` is an unfortunate variable name in Python and is only written to, so can be removed.

💬 purpleKarrot commented on pull request "cmake: Fix `FindQt` module":
(https://github.com/bitcoin/bitcoin/pull/32709#issuecomment-3027803729)
ACK 89d3daf823e4350667b42473c0924430ea7400bc

💬 purpleKarrot commented on pull request "cmake: Fix `FindQt` module":
(https://github.com/bitcoin/bitcoin/pull/32709#discussion_r2180015085)
I don't think upstream considers this as an issue. Whether those variables should be considered advanced or not is a matter of taste.

💬 fanquake commented on pull request "cmake: Fix `FindQt` module":
(https://github.com/bitcoin/bitcoin/pull/32709#discussion_r2180022340)
If anything, that would seem inconsistent, because they have marked every other dependency as advanced, as there's no explanation for why `XKB` hasn't been marked as such (my guess is just oversight).

💬 fanquake commented on pull request "cmake, qt: Process `*.qrc` files manually":
(https://github.com/bitcoin/bitcoin/pull/32648#issuecomment-3027824977)
NACK - this should be fixed in Qts tooling.

👋 fanquake's pull request is ready for review: "build: add `-Wleading-whitespace`"
(https://github.com/bitcoin/bitcoin/pull/32482)

💬 fanquake commented on pull request "build: add `-Wleading-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3027826781)
Dropped `-Wtrailing-whitespace` for now. That could be added when Qts tools are fixed.

💬 fanquake commented on pull request "test: Introduce `SUPPRESS_ABORT_MESSAGE` environment variable":
(https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-3027837451)
What is the status of this?

✅ fanquake closed a pull request: "RFC: build: support for pre-compiled headers."
(https://github.com/bitcoin/bitcoin/pull/31053)

💬 fanquake commented on pull request "RFC: build: support for pre-compiled headers.":
(https://github.com/bitcoin/bitcoin/pull/31053#issuecomment-3027853404)
Closing for now. Could re-open if you want to circle back to looking at this.

✅ fanquake closed a pull request: "bench: reduces unnecessary big vector definition"
(https://github.com/bitcoin/bitcoin/pull/32787)