-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Add C11 API with type-safe formatting (#4663) #4671
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
I'll take a closer look and leave comments inline but first let's rename c.h and c.cc to more descriptive fmt-c.h and fmt-c.cc.
include/fmt/c.h
Outdated
| # include <cstddef> | ||
| # include <cstdint> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed, we should always use C headers here.
| # if defined(_MSC_VER) && (!defined(_MSVC_TRADITIONAL) || _MSVC_TRADITIONAL) | ||
| # error \ | ||
| "C API requires MSVC 2019+ with /Zc:preprocessor flag. Add /Zc:preprocessor to your compiler flags." | ||
| # endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
becuase i used ' VA_ARGS ' and it was causing problems while compiling with legacy compiler so i tested with zc:preprocessor and it worked ...
it need VA_ARGS becuz of that FMT_MAP uses this to split and apply FMT_MAKE_ARG to each one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need this check because the compiler will already give an error if VA_ARGS are not supported. Let's remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure , i'll remove it.
Just to clarify - the legacy preprocessor technically 'supports' VA_ARGS but expands them incorrectly...effectively as a single token , which causes cryptic syntax errors downstream rather than a clear 'unsupported' error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Could you give a godbolt example illustrating the cryptic error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://godbolt.org/z/dcd83Goz3
but if we remove the compiler flags the count comes 1
src/c.cc
Outdated
| #include <string> | ||
| #include <vector> | ||
|
|
||
| static const size_t MAX_PACKED_ARGS = FMT_C_MAX_ARGS; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed because FMT_C_MAX_ARGS can be used directly.
src/c.cc
Outdated
| static const size_t MAX_PACKED_ARGS = FMT_C_MAX_ARGS; | ||
|
|
||
| extern "C" { | ||
| static thread_local std::string g_last_error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't use mutable globals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used it - so that fmt_get_c_error can retrieve it ......
but sure , i will change it
just a quick question to remove it should i go with a optional buffer in format functions or just error codes will be enogh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error codes should be enough.
include/fmt/c.h
Outdated
| # define FMT_MAP(f, ...) \ | ||
| FMT_CAT(FMT_MAP_, FMT_NARG(__VA_ARGS__))(f, ##__VA_ARGS__) | ||
|
|
||
| # define fmt_snprintf(buf, cap, fmt, ...) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename to fmt_format since it uses a different syntax from printf.
include/fmt/c.h
Outdated
| # define fmt_fprintf(f, fmt, ...) \ | ||
| fmt_c_print( \ | ||
| f, fmt, \ | ||
| (FmtArg[]){{FMT_INT}, FMT_MAP(FMT_MAKE_ARG, ##__VA_ARGS__)} + 1, \ | ||
| FMT_NARG(__VA_ARGS__)) | ||
|
|
||
| # define fmt_printf(fmt, ...) fmt_fprintf(stdout, fmt, ##__VA_ARGS__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not include these in the initial implementation and just focus on formatting to a memory buffer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve implemented the requested changes.
For context, these features were already functioning correctly before the update.
Let me know if there’s anything else you’d like me to revise.
Thanks.
|
sure , i will implement the necessary changes |
Issues #4663
Summary
Implements a type-safe C11 API for {fmt} using
_Genericmacros and a fixed-size argument array as discussed in #4663.Design
_Genericmacros (FMT_MAKE_ARG) automatically map C types to tagged unionArchitecture:
C type → FmtArg → fixed array → basic_format_arg → vformatFeatures
fmt_printf,fmt_fprintf,fmt_snprintf)Testing
40+ test cases covering:
Platform Support
/Zc:preprocessorflag (compile-time check included)