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.

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

Use {fmt} for all string formatting #1847

Merged
merged 33 commits into from
Jan 7, 2020
Merged

Use {fmt} for all string formatting #1847

merged 33 commits into from
Jan 7, 2020

Conversation

ZedThree
Copy link
Member

Another big PR late on a Friday, sorry!

We now use fmt for all our string formatting, instead of the printf-style formatting. This affects calls to Output, BoutException/ParseException, DataFile, OptionsReader, and MsgStack/TRACE. Run bin/bout-v5-format-upgrader.py on your physics models to update them.

One big benefit of fmt is that {} will format any standard type correctly, and we can customise it to work with our types too. This PR uses the more specific {:s}, {:e} syntax because the upgrader tool preserves and corrects the existing specifier.

This will absolutely certainly cause conflicts with everything, but after merging next (either into this, or after this is merged, into your branch) running bin/bout-v5-format-upgrader.py on the affected files should fix things!
You may also need to run that tool on any files you've changed.

One issue is that fmt will accept, say, %s in a string, but won't format it, and will happily take more variables than format specifiers in strings. So we might need to be careful there.

@ZedThree ZedThree added the breaking change Introduces a change that is not backward compatible with the previous major version label Nov 22, 2019
@ZedThree ZedThree added this to the BOUT-5.0 milestone Nov 22, 2019
Copy link
Contributor

@dschwoerer dschwoerer left a comment

Choose a reason for hiding this comment

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

One issue is that fmt will accept, say, %s in a string, but won't format it, and will happily take more variables than format specifiers in strings. So we might need to be careful there.

So one can use %s?

Right now gcc warns (if I remember correctly) if the string modifiers do not match the arguments. So this would mean we get less warnings then we do now?

configure.ac Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
examples/6field-simple/elm_6f.cxx Show resolved Hide resolved
@ZedThree
Copy link
Member Author

ZedThree commented Dec 2, 2019

So one can use %s?

Sorry, I wasn't clear: %s is not a valid format specifier for the new fmt-formatting. The problem is if a user passes a printf-style format string, we can't* catch or warn about it. Hopefully the python tool included here will fix them all though.

There is a macro, FMT_STRING which does compile-time checking of format strings, otherwise it's runtime checking. I'm not sure how this interacts with internationalisation though.


* Actually, we could, as fmt does have fmt::sprintf, but it's ugly and expensive. We'd need to format everything at least twice, once with the printf-style and once with the fmt-style, and then see which one we need to use.

locale/zh_CN/libbout.po Outdated Show resolved Hide resolved
@bendudson
Copy link
Contributor

Thanks @ZedThree ! I think this is a slightly painful change, but it has some really nice advantages and is a big improvement.

bendudson and others added 7 commits December 6, 2019 18:16
Positional arguments used a POSIX extension e.g `%2$s` to re-order arguments.
This is converted to e.g. `{1:s}`. Note that whereas the printf numbering is
from 1, fmt numbering is from 0.
`%s` -> `{:s}`
`%d` -> `{:d}`
Was relative path from BOUT_TOP, but BOUT_TOP was not expanded as
expected. Instead, set FMT_INCLUDE_PATH variable in all required
places which is set to absolute path at configure time
@ZedThree
Copy link
Member Author

The failing tests in the push build are fixed in next, which is why the PR tests are passing.

@ZedThree ZedThree merged commit 37a332b into next Jan 7, 2020
@ZedThree ZedThree deleted the add-fmtlib branch January 7, 2020 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Introduces a change that is not backward compatible with the previous major version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants