💬 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
...

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756557857)
Well, I promise to review it. If you don't believe yourself that it is going to be merged, then maybe it isn't worth it in the first place. Closing for now. This can be a follow-up, or separate pull.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756561525)
I'll leave this as-is for now. GitHub is already hiding the diff for me, so I'll close this for now. Discussion can continue in the follow-up, which will also make it easier to follow.

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756561875)
I understand that, but why are we making sure that the validation allows nonsensical values?

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756562372)
Ok, looking forward to the follow-ups