-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Bcc32 compatibility #1079
Bcc32 compatibility #1079
Conversation
Hi @starkmapper, thanks for the PR! Since this is your first contribution to {fmt}, please review CONTRIBUTING.md, particularly the part about licensing, and comment here whether you agree with it being applied to your contributions including this PR. |
I have read contributing.md and agree that my contribution will be licensed under the fmt license. |
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.
Left some comments in the first commit. Also instead of fixing issues in the follow-up commits, please amend the ones earlier in the stack for easier review and sensible revision history.
I didn't want to rebase+squash just yet, but let me know if that would help you. |
I'm also a little worried about the CI failing, is there anything I can do about that? |
That would help.
Please check the error at https://travis-ci.org/fmtlib/fmt/jobs/507890085. Looks like they are mostly namespace-related. |
8915b27
to
70aacd4
Compare
Squashed and fixed non-windows builds. I'm not sure what's going on with the appveyor build. Did I break that or is something else going on? |
70aacd4
to
b272608
Compare
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 think the Appveyor build is failing because the cmake command in https://github.com/fmtlib/fmt/blob/4.x/support/appveyor-build.py#L11 is missing ".."
at the end for some reason. Could you add it by any chance while at it? Also a few comments inline.
fmt/format.h
Outdated
@@ -2593,11 +2687,8 @@ class SystemError : public internal::RuntimeError { | |||
throw fmt::SystemError(errno, "cannot open file '{}'", filename); | |||
\endrst | |||
*/ | |||
SystemError(int error_code, CStringRef message) { | |||
init(error_code, message, ArgList()); | |||
} |
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 keep this ctor.
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.
It is generated by the FMT_VARIADIC_ macro.
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.
It's here for documentation purposes (note the apidoc above). If it is causing problems for bcc, I suggest conditionally compiling instead of removing.
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.
It's not a bcc issue, it's because all FMT_VARIADIC_*
macros now use the FMT_VARIADIC_
macro, this normalizing variadic template handing, and less macro magic to maintain. The FMT_VARIADIC_
macro generates functions with 0..15 templated arguments to mimic variadic templates, where the 0 argument one has the exact same signature as this manually defined one.
I propose to make it consistent with functions printf
, format
etc and add an overload with an extra ArgList
parameter:
SystemError(int error_code, CStringRef message, ArgList args) {
init(error_code, message, args);
}
This would be the best of both worlds: singe FMT_VARIADIC_
macro and documentation (and more consistency as a bonus :-) ).
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.
If this is not bcc issue, could you remove the macro changes from the current PR?
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.
The need for the macro changes is because of a bcc issue. If you feel very strongly about not changing the constructor signature in the documentation I can change the FMT_VARIADIC_CTOR
macro not to generate the conflicting overload.
Background
The original macro did c-style array initialization:
ArgArray<2>::Type array = { MakeValue<BasicFormatter<char>>(v0),
MakeValue<BasicFormatter<char>>(v1)};
Where ArgArray<2>::Type
is an array of Value
.
Somehow bcc tried to assign the results of MakeValue<BasicFormatter<char>>(vN)
to the first member of Value
(int int_value
in the anonymous union) causing a "Cannot convert `MakeValue<BasicFormatter>(vN) to int" error.
When I started fixing this issue I found the FMT_VARIADIC_
function and started using that. This had already solved this issue by generating the individual assignment statements for the array:
ArgArray<2>::Type arr;
arr[0] = MakeValue<BasicFormatter<char>>(v0), arr[1] = MakeValue<BasicFormatter<char>>(v1);
note: I left out namespaces for clarity
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 see. The macro logic is fairly complicated and version 4.x is in maintenance mode, so I'd like the changes in this PR to be minimal, just enough for bcc32 compatibility. Right now it's hard to see what actually changed because the macros have been moved around. Could you return them to their original places and make changes there?
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 started doing that, but found that the solution would depend on the (newer and more versatile) macros defined later on in the file. That is why I moved the large macros block up. To verify that I only moved them I used kdiff3 with manual alignment. I'll post those diffs so you can get a better idea of the changes I made.
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.
Here you can see what I changed to get this working for bcc with the existing macros. Not only achieving bcc compatibility, but also removing the second set of macro's, resulting in less "magic" to maintain:
And the diff for the FMT_MAKE_REF
macro's:
The alignment of the diff is shown below. I marked the manual alignment markers with red.
I hope this makes it clear that I only made minor changes, apart from moving the macros.
I'll make the proposed changes. Would you like me to squash again, or just push updates? Also, should I check the documentation using doxygen, or apidoc like you mentioned? |
Adding the |
1758351
to
b78b305
Compare
But adding |
Thanks for fixing this.
Yes, please squash and remove the macro changes as discussed above.
I don't think it's necessary. |
6ef7a21
to
57a94f7
Compare
Resolve namespace issues Add workaround for compile error on bool template argument of ArgArray struct Squelch bcc32 warning on accessing the digits array Squelch bcc32 warning on unused values Use FMT_VARIADIC_ for FMT_VARIADIC_VOID and FMT_VARIADIC_CTOR Fix warnings about redefinig macros and conditions always true Disable "LConv" block for bcc32 compiler Remove macro test for deprecated macro Fix appveyor-build for cmake v3.13+
57a94f7
to
159e12c
Compare
I changed the macros so the |
Merged everything except the macro changes in d2744bc. The macros need a bit more work because they are in their present location for a reason: to make the |
I'll try to find a less invasive way to make the macros work. |
I did some work to get fmt working on the "classic" Embarcadero compiler.