💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756495304)
Thanks, the diff looks good, but would you mind if you submitted it in a follow-up? I presume you ran it locally, and it didn't uncover any bugs in this pull request, so that is a useful signal already. However, at some point I want to wrap up with this pull request myself. There are already 150 comments, most of which are hidden by GitHub, and thus almost impossible to navigate.

If I included your diff, someone may suggest to "reduce verbosity" by using the compiler to construct the args of
...

🚀 fanquake merged a pull request: "build: Minor build system fixes and amendments"
(https://github.com/bitcoin/bitcoin/pull/30803)

🤔 l0rinc requested changes to a pull request: "util: Use consteval checked format string in FatalErrorf, LogConnectFailure"
(https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2299632852)
It seems to me we still have a remaining trailing formatter bug that we should fix before pushing

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756421832)
Some tense changes could clarify the exact behavior:
```suggestion
decltype(fmt)::Detail_CheckNumFormatSpecifiers(fmt.fmt); // This was already executed at compile-time, but is executed again at run-time to avoid -Wunused.
```

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756422993)
What does `NormallyPassFmt<sizeof...(Args)> would be used.` mean exactly?

```suggestion
// args directly. NormallyPassFmt<sizeof...(Args)> would be used.
```

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756483366)
Thanks, updated the example

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756493521)
We could standardize this by extending `HasReason` in `setup_common.h`:
```C++
class HasReason
{
public:
explicit HasReason(const std::string_view& reason) : m_reason(reason) {}
bool operator()(std::string_view s) const { return s.find(m_reason) != std::string_view::npos; }
bool operator()(const std::exception& e) const { return (*this)(e.what()); }

private:
const std::string_view m_reason;
};
```
resulting in
```C++
BOOST_CHECK_EXCEPTION(util::ConstevalFormatStri
...

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756489714)
This seems like an error to me, shouldn't trailing `%` be invalid? It fails for `"%123 %"`, shouldn't it fail for these as well?
The reason for the misclassification is https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754908585, i.e. that when detecting a `%` we skip the `if (++it >= str.end())` check.

The example in https://github.com/bitcoin/bitcoin/pull/30546/files#r1754643534 detects this as an error

🤔 l0rinc requested changes to a pull request: "util: Use consteval checked format string in FatalErrorf, LogConnectFailure"
(https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2299744441)
It seems to me we still have a remaining trailing formatter bug that we should fix before pushing

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756500154)
> Otherwise, it will never be ready because there is always a way to come up with an idea to improve something

I think we're getting closer, your patience is appreciated :)

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756503078)
Ah, it should say "Normally PassFmt<sizeof...(Args)> would be used." (in code)

💬 hodlinator commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756508500)
Yeah, I can do it as a follow-up since by now it does not really modify your existing code that much.

If you need to re-touch, I would appreciate if you just took this diff:
```diff
- PassFmt<129>("%129$s 999$s %2$s");
+ PassFmt<12>("%12$s 999$s %2$s");
```
to make the follow-up cleaner.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756514460)
Please take a look at the line above your comment. It says `tinyformat accepts almost any "type" spec, even '%', or '_', or '\n'`.

If you don't believe the comment, and the compile time check, you can try for yourself: https://godbolt.org/z/n6ToecPKT

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756518642)
I don't want to modify `HasReason` here. There are already more than 150 comments and increasing the scope won't help. Maybe in a follow-up, or separate pull?

💬 maflcko commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2345782247)
Needs rebase and CI failures fixed (they are true ones)

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756526361)
I have of course seen the comment and believe it, but I don't think we should mandate `"%123%"` being correct (i.e. not just allowing it, but adding test for it), while disallowing `"%123 %", "%" and "%1$".

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756527665)
Ok, though I doubt it will get merged if it's in a separate PR...

💬 fanquake commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2345809401)
> What if you ccache --zero-stats after configuring for a new build tree, just before building?

In that case on Fedora it is `Hits: 446 / 446 (100.0%)`, with `Uncacheable calls: 12 / 458 ( 2.62%)`.

🚀 fanquake merged a pull request: "build: Introduce "Kernel" installation component"
(https://github.com/bitcoin/bitcoin/pull/30835)

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756555544)
> while disallowing `"%123 %"`, `"%"` and `"%1$"`.

They are invalid and will cause a runtime error, which is the reason why they are disallowed and the whole point of this pull request.

Apart from that, I don't think it makes sense to spend a massive amount of time on format strings that are POSIX-invalid, but currently happen to be tinyformat-valid. No one should be using them, but if someone uses them it won't cause a runtime error but a harmless formatting error (or no error at all). Bu
...