💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754093556)
nit: personally I'd prohibit positional args completely, they can be super confusing - and shouldn't be needed, we can extract variables before the call if we need to repeat them, instead of the reader being forced to count parameters... but if we do, I have a few suggestions

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754518405)
While it gets the job done, I think the parser is unnecessarily complicated for several reasons:
* Redundant Counters: Separate (but mutually exclusive) `count_normal` and `count_pos`.
* Repetition: The check for "Format specifier incorrectly terminated by end of string" is repeated in multiple places.
* Inefficient Character Handling: Multiple character checks for `%` (and manual digit detection).
* Scattered Iterations: Increment operations (++it), continue statements, and lookaheads (str.
...

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754542178)
I don't particularly like that we have lookaheads even though we should be able to detect this at the very end - which would enable us to remove the duplicated throw.

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754545143)
I find these jumps quite confusing, it doesn't reflect how I would manually read and interpret such a format string

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754239951)
👍, "indicate" may not be the best choice in this context, how about:
```suggestion
if (maybe_num == 0) throw "Positional format specifiers start at 1"};
```

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754535527)
Alternatively, we could cheaply collect the referenced indexes to a `std::set<unsigned> referenced_params;` and check that all of them were referenced, i.e. `if (referenced_params.size() != num_params) throw "Not all parameters are referenced!";`

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754284187)
since we can only have either `count_normal` or `count_pos`, but we also need to detect if both were called, could we separate the two concerns: have a single count which is used in both cases and two booleans, a "has_normal" and a "has_positional"?

💬 stickies-v commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1754555353)
This change was made because of [my suggestion](https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742175853) but you're right there can be a case difference which I did not consider. Sorry for that @l0rinc .

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754581608)
Thanks, done!

(Side note: `count_format_specifiers` also does not implement `*` parsing correctly, which I guess was never detected because it was never used)

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754585565)
Thanks, removed!

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754586257)
Thanks, removed!

💬 hebasto commented on pull request "test: Drop no longer needed workarounds":
(https://github.com/bitcoin/bitcoin/pull/30847#issuecomment-2343712471)
cc @maflcko

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754587654)
Thanks, (re)moved.

💬 l0rinc commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1754587684)
[Done](https://github.com/bitcoin/bitcoin/compare/6da47f6ba5450e7f2b2ab2a64c7cfd354f67a1a9..1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa), thanks both for the suggestions (this is how we get to the best outcome).

💬 l0rinc commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1754588210)
I'm full of surprises - [done](https://github.com/bitcoin/bitcoin/compare/6da47f6ba5450e7f2b2ab2a64c7cfd354f67a1a9..1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa)

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754589089)
Thanks, rephrased!

💬 l0rinc commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1754589421)
[Done](https://github.com/bitcoin/bitcoin/compare/6da47f6ba5450e7f2b2ab2a64c7cfd354f67a1a9..1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa)

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754591363)
Thanks, took it and extended the `@note` a bit.

💬 l0rinc commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1754600019)
I didn't `const` them since they're only used in the next line (i.e. very narrow scoped), seems like noise in this case - but if you insist, I'll add it

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754600910)
They are needed, see https://github.com/bitcoin/bitcoin/pull/30546#issuecomment-2333713098

If you want to prohibit them, a separate pull request may better suited to remove them.