Nuke format linter

maflcko commented at 2:18 pm on July 26, 2024: member

The linter is impossible to maintain, because it is not possible to parse C++ with a simple regex. When it produces nonsense, like https://github.com/bitcoin/bitcoin/runs/27958379841, it is mostly annoying, because the code is perfectly fine, but the red CI is a blocker for the pull request. Also, fixing it isn’t trivially possible, so the only solution is to juggle with the code until the regex in the linter is pleased.

It would be better to write it as a clang-tidy plugin, however that isn’t ideal either, because that will (normally) only run in CI and takes a long time.

Ideally, it is implemented in consteval C++, directly in the source code. (Similar to Rust std::fmt or C++20 std::format, but they aren’t available yet on supported compilers and would be a breaking change in all format strings anyway)

maflcko commented at 8:45 am on July 30, 2024: member

cc @hodlinator This issue should track your concern from #28052#pullrequestreview-2202767978

I’ll open a pull request shortly.

hodlinator commented at 2:39 pm on July 30, 2024: contributor

Thanks for the heads up!

Yeah, regexps seem like a blunt tool for C++ parsing. I guess one could make the case that it just needs to be battle-hardened for a very small subset of it… “good enough”. But after having to trouble-shoot it enough times I understand wanting to remove it.

If we were using variadic non-template arguments, we might be able to make use of GCC’s __attribute__ ((format (printf, …))). Unsure if it handles user defined types or even std::string well though.

http://fmt.dev seems interesting as a potential candidate to replace tinyformat with.. but I’m not suggesting anyone do that. :sweat_smile:

maflcko commented at 3:18 pm on July 30, 2024: member

If we were using variadic non-template arguments, we might be able to make use of GCC’s attribute ((format (printf, …))). Unsure if it handles user defined types or even std::string well though.

Good point about the attribute. There should be no need to support std::string or user defined types. (Should there ever be a need to support them, the checks can be trivially side-stepped in that particular line of code by using the unchecked format function: CheckedFormat("%s", UncheckedFormat(std::string{"%s"}, 1)).

Edit: I checked the attribute and I couldn’t get it to work. See also https://stackoverflow.com/questions/12573968/how-to-use-gccs-printf-format-attribute-with-c11-variadic-templates

http://fmt.dev seems interesting as a potential candidate to replace tinyformat with.. but I’m not suggesting anyone do that. 😅

Right. The benefits would be unclear. If tinyformat was replaced with anything, it would probably be std::format.

hodlinator commented at 9:19 pm on August 30, 2024: contributor

I get fmt.dev would be challenging to add as a dependency (makes me think of log4j :smiling_face_with_tear:). std::format appears heavily influenced by (if not authored by?) the creator of fmt.dev. But yeah, migrating to fmt.dev temporarily to then migrate again to std::format is just extra churn anyway.

While looking over #30546 I started to look for fmt.dev’s printfimplementation only to realize it might force me to include some extra copyright notice due to their (variant of?) MIT license. Didn’t get to inspecting the actual code though.

C++20 std::format, but they aren’t available yet on supported compilers and would be a breaking change in all format strings anyway

Was surprised at first because I assumed all of C++20 was available. But comparing https://en.cppreference.com/w/cpp/compiler_support/20#C.2B.2B20_library_features “Text formatting” with /doc/(dependencies|build-windows-msvc).md does indeed show us coming up short. :\

ryanofsky referenced this in commit e46bebb444 on Sep 12, 2024

maflcko commented at 7:53 am on September 13, 2024: member

The next step is https://github.com/bitcoin/bitcoin/pull/30889

Nuke format linter #30530