Conversation
Ozaq
force-pushed
the
feature/add-libfmt
branch
from
e4a62ce to
5ffccda
Compare
|
Private downstream CI failed. |
Ozaq
force-pushed
the
feature/add-libfmt
branch
from
5ffccda to
4b6d2f6
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #182 +/- ##
===========================================
- Coverage 65.77% 64.11% -1.67%
===========================================
Files 1131 1144 +13
Lines 57406 59883 +2477
Branches 4337 4645 +308
===========================================
+ Hits 37756 38391 +635
- Misses 19650 21492 +1842 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Ozaq
force-pushed
the
feature/add-libfmt
branch
3 times, most recently
from
d46cff7 to
f4a90c7
Compare
|
Private downstream CI failed. |
|
Private downstream CI failed. |
|
(Not a review) This is an ideal candidate for git submodules. You can pin to a version, a branch typically main/master. Don't discard the testing.... |
|
Private downstream CI failed. |
|
Private downstream CI failed. |
pgeier
force-pushed
the
feature/add-libfmt
branch
from
83085b6 to
d89bddd
Compare
|
Private downstream CI failed. |
|
Private downstream CI failed. |
|
Private downstream CI failed. |
pgeier
force-pushed
the
feature/add-libfmt
branch
from
23e0ccb to
d89bddd
Compare
|
Private downstream CI failed. |
|
Private downstream CI failed. |
|
Private downstream CI failed. |
|
Private downstream CI failed. |
|
Private downstream CI failed. |
|
Private downstream CI failed. |
|
Private downstream CI failed. |
|
So the find_dependency stuff is working properly now - also from a bundle. Now we just need to see why the pyfdb runner with gnu8.5 is using the lib64/ instead of lib/ path |
|
Private downstream CI failed. |
|
Private downstream CI failed. |
|
In a separate PR I'm proposing to use git submodules as a way to integrate third-party libraries functionality without copying and with direct access to said libraries versioning. We already have the example of using xxHash, and for libfmt I'm of the opinion of not copying code into eckit, especially if that means stripping functionality (documentation, testing). if you find you have opinions on this please add yourself to review the PR! |
|
Private downstream CI succeeded. |
|
Private downstream CI succeeded. |
|
Private downstream CI succeeded. |
16 similar comments
|
Private downstream CI succeeded. |
|
Private downstream CI succeeded. |
|
Private downstream CI succeeded. |
|
Private downstream CI succeeded. |
|
Private downstream CI succeeded. |
|
Private downstream CI succeeded. |
|
Private downstream CI succeeded. |
|
Private downstream CI succeeded. |
|
Private downstream CI succeeded. |
|
Private downstream CI succeeded. |
|
Private downstream CI succeeded. |
|
Private downstream CI succeeded. |
|
Private downstream CI succeeded. |
|
Private downstream CI succeeded. |
|
Private downstream CI succeeded. |
|
Private downstream CI succeeded. |
simondsmart
left a comment
simondsmart
left a comment
There was a problem hiding this comment.
I'm very happy with this, modulo the comment that I don't understand why we are creating essentially a second contrib directory. Please include the code in the same way as other contexts!
| clang_format: true | ||
| clang_format_ignore: | | ||
| src/eckit/contrib | ||
| third-party |
There was a problem hiding this comment.
Why are we creating a second directory here. I don't see libfmt as a dependency being conceptually meaningfully different to xxHash. They are both third party contribs. Please put in the 'original' location.
There was a problem hiding this comment.
well, it is a third-party library ... same for xxHash. The naming contribution is just not appropriate for these because they are no contributions from externals provided just for this purpose.
Also note that in #187 it is to discussed to move them to third_party directory as well, and also use git submodules properly...
However, we want to keep the discussion with the git submodules separate. That's why this library is still vendored.
There was a problem hiding this comment.
Semantics... contrib is the directory for external contribution; personally I never heard of the term "vendored" before in this context either.
If instead this is agreed to go to a new directory third-party, then I propose to also move xxHash to this third-party to be consistent at least.
| #include "eckit/exception/Exceptions.h" | ||
| #include "eckit/types/FloatCompare.h" | ||
|
|
||
| using std::fpclassify; |
There was a problem hiding this comment.
This seems unrelated.
We weren't using the std:: ones as we had issues on some compiler combinations. Not sure I want to couple this change in this context?
There was a problem hiding this comment.
we included the eckit/format/Format.h in eckit/log/Log.h - this was then included in FloatCompare
Here some macros to math functions were used to avoid having to prefix std:: ... however as they were declared as macros, they caused a lot of conflicts with the included libtfmt headers.
In that sense using the using std::function; approach is much cleaner. I'm not sure if this approach was properly tested before? Moreover we are now using C++17 - I'd be really supprised if there are still compilers complaining about these things. Also the CI passes.
wdeconinck
left a comment
wdeconinck
left a comment
There was a problem hiding this comment.
I think this is a nice addition, thanks @Ozaq !
I wonder the usefulness of adding these macros currently:
- eckit_format
- eckit_format_cc
- eckit_format_to
- eckit_format_to_cc
Adding macros should be avoided, and I understand these can improve performance from the comments, but I don't think we really need to worry about this. So far we have managed without this. Logging should not be in a critical loop anyways.
If this appears to be a requirement we could add this later?
Then, I don't know why it was chosen to call the new functions eckit::str_format (instead of eckit::format) and eckit::str_format_to (instead of eckit::format_to) rather than the similar names in libfmt and C++20 std::format and std::format_to.
Naming the functions similar will be helpful to port later to C++20.
Is the ENABLE_FORMAT procedure similar for libfmt and C++20's format?
Looking at this blogpost it looks a bit different, but I have not investigated further.
| * For most cases it is encouraged to use the macro `eckit_format`. It will perform compile time checks. | ||
| * | ||
| * For a very specific timecritical cases `eckit_format_cc` can be used to produce very optimized formatting code. | ||
| * Disadvantag: more binary code |
There was a problem hiding this comment.
| * Disadvantag: more binary code | |
| * Disadvantage: more binary code |
| #define ENABLE_FORMAT(typ) \ | ||
| template <> \ | ||
| struct fmt::formatter<typ> : fmt::ostream_formatter {} |
There was a problem hiding this comment.
This macro should not be so generic but be prefixed with ECKIT_
The function |
ugh... bummer. |
About The compiletime checks guard against type mismatches and providing additional arguments to the format call. E.g. format("just a single value: {}", 1 ,2) would not compile, if this is done at runtime an exception would be thrown. I believe once we have format available this will be used a lot when building messages for exceptions, e.g. "Oh noes x[{}] != y[{}]". Unless all these error code paths are executed they might fail at runtime and instead of throwing the exception we want, we end up with a fmt exception about faulty formatting instead. I am personally against providing eckit wrappers for fmt but this was what I understood as a requirement. I would consider the effort to manually update from About These two macro are primarily concerned with performance and offer a compiletime/codesize vs. runtime allocation behavior tradeoff. They are primarily added to highlight that they exist and enable to log from some somewhat performance critical code (apply your own judgment, Its sill not free even if cheaper) And the PR is still at this point a basis for discussion.
Because it clashes with
What is happening here is that we are using the fmt ostream_formatter (https://fmt.dev/latest/api/#stdostream-support), a formatter that is designed to make types that support the stream oper already printable with format. This fallback does not exist in C++20/23. On migration to std::format (independently of fmt) we would need to add formatters for all types (that we want to print, so a good chance is that this means for all types that already have a ostream oper) as described in the link you added. |
This commit also provides new logging macros in Log.h NOTE: This should not be merged as-is, this is a basis for discussion Co-authored-by: Philipp Geier <philipp.geier@ecmwf.int>
Ozaq
force-pushed
the
feature/add-libfmt
branch
from
8150253 to
6571e59
Compare
tbkr
left a comment
tbkr
left a comment
There was a problem hiding this comment.
Check with the other reviewers first.
7 participants
No description provided.