Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Codechange: use the fmt library for simpler debug formats #9352

Merged
merged 5 commits into from
Jun 13, 2021
Merged

Codechange: use the fmt library for simpler debug formats #9352

merged 5 commits into from
Jun 13, 2021
+9,545 −643

Conversation

rubidium42
Copy link
Contributor

@rubidium42 rubidium42 commented Jun 11, 2021 •
edited
Loading

Motivation / Problem

The format codes for printf are complicated and annoying to work with. When changing something from C-string to std::string you need to explicitly add .c_str(), but also for some types you need to "inject" PRINTF_SIZE.

Preferably we would use something in the standard library, but formatting has landed in C++20 so... almost there but not quite yet. There is however the "fmt" library that has the same API as C++20 just in a different namespace, so we can use that as a stop-gap measure until we go to C++20. See https://fmt.dev/7.1.3/api.html#compatibility-with-c-20-std-format

Description

The vendoring of the "fmt" library and full conversion of all DEBUG statements to Debug.

Limitations

  • ~9000 lines of extra code due to the vendoring of the library.
  • There is a set of fmt::printf functions, however they do not support things like %ld so using that as a migration step is out of the question.
  • The error messages on wrong formats can be very unhelpful, though it does have compile time validation of the format strings.
  • There are a lot of inconsistencies in the debug messages. Those are not in scope here, unless someone wants to patch that on top of this. That I'll gladly accept.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@glx22
Copy link
Contributor

glx22 commented Jun 11, 2021

Ok, for targets using vcpkg you can add fmt to the install list. For MinGW it's also fmt. Don't know for Linux and Emscripten.

@LordAro
Copy link
Member

LordAro commented Jun 11, 2021

Isn't it a header-only library anyway? Might as well just vendor it

@rubidium42
Copy link
Contributor Author

Ok, for targets using vcpkg you can add fmt to the install list. For MinGW it's also fmt. Don't know for Linux and Emscripten.

I tried looking for that, but couldn't find it in the source code. So I guess it is in some helper repository. In any case, I have not bothered with that as the decision for "vendoring" it or not needs to be made first. If we are going to vendor it, then adding the library to those lists has been quite futile.

Isn't it a header-only library anyway? Might as well just vendor it

You can use it as header-only library or as normal library. For the former you need to add an extra define before the include, though that should not be a problem.

@rubidium42 rubidium42 force-pushed the fmt-for-debug branch 3 times, most recently from cb86294 to 8e86f8c Compare June 12, 2021 10:05
@rubidium42 rubidium42 marked this pull request as ready for review June 12, 2021 10:25
TrueBrain
TrueBrain reviewed Jun 13, 2021
View reviewed changes
Copy link
Member

@TrueBrain TrueBrain left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would make life so much better in many ways (how often I get warnings because I used %d instead of %lu .. I cannot even start to tell you about).

PS: I vote to vendor this library; it is small enough after all.
PPS: your description is pretty much out-of-date :P

@rubidium42 rubidium42 merged commit 7d79180 into OpenTTD:master Jun 13, 2021
@rubidium42 rubidium42 deleted the fmt-for-debug branch June 13, 2021 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michicc michicc

@TrueBrain TrueBrain

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@rubidium42 @glx22 @LordAro @michicc @TrueBrain