Page MenuHomeWildfire Games

Updated cppformat
Needs RevisionPublic

Authored by adrian on Sun, Feb 2, 10:51 PM.

Details

Reviewers
Itms
Angen
Trac Tickets
#5646
Summary

I updated cppformat to version 1.1.0 (first stable version after bug fixed) and changed its name to fmt (like github repository). All previous modifications are preserved.

Test Plan

Just compile and run tests :)

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

adrian created this revision.Sun, Feb 2, 10:51 PM
asterix added a subscriber: asterix.

Thank you much for this patch. I suspect Adrian that you will further update it until recent release (especially those optimizations)?

Stan added a subscriber: Stan.Mon, Feb 3, 12:27 PM

Hey,

Can you reupload the patch so it can go through CI? It didn't go because you weren't added to the Contributors group.

Itms added a comment.Mon, Feb 3, 2:25 PM

Thank you for the patch! We've been meaning to update cppformat (now fmtlib) for some time now. I had a half-finished patch for going to 2.1.1 (which was the last update before the renaming) in #3190, then was planning to update to the newest version in #4148.

However, your proposal to fix the bug in #5646 and use the opportunity to rename the lib also makes sense. It should then be easier to update to recent versions of the lib. Will you be interested in working on that after #5646 is fixed?

One issue we had with respect to upgrading cppformat is that it's not very clear which changes are from the upgrade, and which ones are from us for 0ad. Could you generate a diff between the upstream 1.1.0 version and the one you uploaded here? I think it should be possible and clean enough to create a source/third_party/fmt/patches/ directory in which we store patches for the changes we made. It would help with readability of our changes, and with sending fixes upstream if needed.

And thanks for the ping @asterix ๐Ÿ‘

adrian added a comment.Sun, Feb 9, 9:01 PM

Thanks a lot for the comments!

The problem with further updates is a lot of changes in fmt. Fmt version 6.x has completely different api in comparison to cppformat. Update to v1.1.0 wasn't very difficult task, but update to v6.x is much harder.
I propose to merge this changes into codebase (I can restore old cppformat name) and create another ticket to update fmt to version 6.x. Unfortunately I'm not sure how much time I will have in the future, so I think better option is to not "lock" this issue further.

@Stan How to upload once again? Do I need to create a new review or just edit this? This is my first time here :)

Itms added a comment.Sun, Feb 9, 10:24 PM

I propose to merge this changes into codebase (I can restore old cppformat name) and create another ticket to update fmt to version 6.x. Unfortunately I'm not sure how much time I will have in the future, so I think better option is to not "lock" this issue further.

I agree, and actually I think it's very nice to use this small upgrade to rename the lib, so you can leave it like this.

However I'd still really like us to start versioning our changes to the lib, by adding a diff file containing the differences between upstream's 1.1.0 and this version, to our repo.

@Stan How to upload once again? Do I need to create a new review or just edit this? This is my first time here :)

Updating the revision with a new diff (for instance including the patch file I'm talking about) will have the effect wanted by Stan :)

adrian updated this revision to Diff 11321.Mon, Feb 10, 6:51 PM
Itms set the repository for this revision to rP 0 A.D. Public Repository.Mon, Feb 10, 7:04 PM

(you also needed to specify the repository in order to trigger a build, I fixed that)

adrian added a comment.EditedMon, Feb 10, 7:18 PM

Thanks!

Diff between 0ad and original version. Fortunately it's not so complicated.

1a2,6
>  * Slightly modified version of fmt, by Wildfire Games, for 0 A.D.
>  * Based on fmt v1.1.0 from https://github.com/fmtlib/fmt
>  */
> 
> /*
40d44
< #include <string>
43c47,49
< #if _SECURE_SCL
---
> #include "ps/CStr.h"
> 
> #if defined(_SECURE_SCL) && _SECURE_SCL
131a138,140
> # if defined(_MSC_VER)
> #  define FMT_USE_VARIADIC_TEMPLATES 1
> # else
134c143,144
<        (FMT_GCC_VERSION >= 404 && FMT_HAS_GXX_CXX11) || _MSC_VER >= 1800)
---
>        (FMT_GCC_VERSION >= 404 && __cplusplus >= 201103))
> # endif
141a152,153
> # elif defined(_MSC_VER)
> #  define FMT_USE_RVALUE_REFERENCES 1
145c157
<         (FMT_GCC_VERSION >= 403 && FMT_HAS_GXX_CXX11) || _MSC_VER >= 1600)
---
>         (FMT_GCC_VERSION >= 403 && __cplusplus >= 201103))
154c166
< #if FMT_USE_NOEXCEPT || FMT_HAS_FEATURE(cxx_noexcept) || \
---
> #if (defined(FMT_USE_NOEXCEPT) && FMT_USE_NOEXCEPT) || FMT_HAS_FEATURE(cxx_noexcept) || \
163c175
< #if FMT_USE_DELETED_FUNCTIONS || FMT_HAS_FEATURE(cxx_deleted_functions) || \
---
> #if (defined(FMT_USE_DELETED_FUNCTIONS) && FMT_USE_DELETED_FUNCTIONS) || FMT_HAS_FEATURE(cxx_deleted_functions) || \
290c302
< #if _SECURE_SCL
---
> #if defined(_SECURE_SCL) && _SECURE_SCL
486c498
< #if _SECURE_SCL
---
> #if defined(_SECURE_SCL) && _SECURE_SCL
871a884
>             FMT_MAKE_STR_VALUE(const CStr &, STRING)
1570c1583
< #if _SECURE_SCL
---
> #if defined(_SECURE_SCL) && _SECURE_SCL
2077c2090
< #if _MSC_VER
---
> #if defined(_MSC_VER) && _MSC_VER

@Itms what should be the next step? :)

Itms added a comment.Wed, Feb 12, 9:29 PM

I will test this and hopefully commit it during the upcoming weekend. I am a bit short on free time these days, so unfortunately when things are related to libraries some of the steps include waiting for me. Don't hesitate to contribute to other areas of the code in the meantime. Or if you want you can start working on the future cppformat upgrade steps since they seem to involve a lot of work.

Angen resigned from this revision.Thu, Feb 13, 9:46 PM

No problem. Thanks a lot!

Itms requested changes to this revision.Mon, Feb 17, 8:05 PM

Hi! Things look good to me ๐Ÿ‘

A few comments/nitpicks before I commit:

  • Can you add the diff to the commit, in unified style, as a file source/third_party/patches/fmt.diff (in the future, I'll post inline comments directly there...)
  • I don't know why we disable warnings outside the if _MSC_VER block, as those pragma instructions only work on MSVC. I think we can just add disabling of warning 4456 to the upstream version of format.cpp
  • Can you update the copyright year of test_fmt.h
  • We don't have to keep __cplusplus >= 201103 preprocessor conditionals, we can follow upstream and use FMT_HAS_GXX_CXX11
  • Can you harmonize the indentation for FMT_MAKE_STR_VALUE(const CStr &, STRING), so that it's the same as the previous lines (easier to spot in the unified diff with some lines of context)

If any comment I made is not clear, please still upload the patch containing the unified diff and I'll make inline comments on it ๐Ÿ™‚

This revision now requires changes to proceed.Mon, Feb 17, 8:05 PM