👋 fanquake's pull request is ready for review: "[28.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/32811)
(https://github.com/bitcoin/bitcoin/pull/32811)
🚀 fanquake merged a pull request: "build, docs: Fix Boost-related issues on NetBSD"
(https://github.com/bitcoin/bitcoin/pull/32828)
(https://github.com/bitcoin/bitcoin/pull/32828)
💬 fanquake commented on pull request "refactor: Drop unused `#include <boost/operators.hpp>`":
(https://github.com/bitcoin/bitcoin/pull/32668#issuecomment-3027713936)
Yea, I'm not sure what is the best approach here. However I don't think a good way forward is PR's like this, based on IWYU output, which simultaneously ignore other IWYU output (and no explanation for devs that might otherwise try to make changes based on the output). I think any changes should be a part of #31308.
(https://github.com/bitcoin/bitcoin/pull/32668#issuecomment-3027713936)
Yea, I'm not sure what is the best approach here. However I don't think a good way forward is PR's like this, based on IWYU output, which simultaneously ignore other IWYU output (and no explanation for devs that might otherwise try to make changes based on the output). I think any changes should be a part of #31308.
💬 fanquake commented on pull request "test: fix feature_init.py intermittencies":
(https://github.com/bitcoin/bitcoin/pull/32835#issuecomment-3027724136)
cc @hodlinator
(https://github.com/bitcoin/bitcoin/pull/32835#issuecomment-3027724136)
cc @hodlinator
💬 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.
(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
```
(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.
(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.
(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?).
(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.
(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()`.
(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
(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
```
(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?
(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.
(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
(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.
(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).
(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.
(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)
(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.
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3027826781)
Dropped `-Wtrailing-whitespace` for now. That could be added when Qts tools are fixed.