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

Allow debug prints without __VA_ARGS__ in non-MSVC #322

Merged
merged 1 commit into from
Mar 31, 2020
Merged

Allow debug prints without __VA_ARGS__ in non-MSVC #322

merged 1 commit into from
Mar 31, 2020

Conversation

hemmick
Copy link
Contributor

@hemmick hemmick commented Nov 5, 2019

The terminating comma does not get removed in non-MSVC compilers. Use ## to consume the comma in this case, allowing zero-arg prints

@geky
Copy link
Member

geky commented Nov 14, 2019

Hi @hemmick, thanks for creating a PR. Unfortunately this doesn't work in standard c99 which means if we start adopting zero-arg prints we'll start writing code that can't be ported to other standard compilers.

So the current solution is to avoid zero-arg prints. It's not a good solution but it's the only solution the standard offers us.


That was the current state of things, but it's been getting more and more annoying in a few other projects I've been working on so I looked around for more info and did find one portable hack that would get around this. It's not the greatest, but it does have the expected semantics:

#ifndef LFS_NO_DEBUG
#define LFS_DEBUG_(fmt, ...) \
    printf("waka_debug:%d: " fmt "%s\n", __LINE__, __VA_ARGS__)
#define LFS_DEBUG(...) \
    LFS_DEBUG_(__VA_ARGS__, "")
#else
#define LFS_DEBUG(fmt, ...)
#endif

I think that's what we should go with. And if the ##__VA_ARGS__ is available users can override the LFS_DEBUG declaration with their own locally.

We could detect some compilers and use ##__VA_ARGS__ where available, but messing around with this in GCC I kept getting a "ISO C99 requires at least one argument" warning. Searching around as far as I can tell this is impossible to disable with -pedantic, which is too useful to turn off.

It's still possible for users to override LFS_DEBUG locally if they're compiling without -pedantic, so I think this is the best solution available to us.

@dpgeorge
Copy link
Contributor

FWIW, the way we deal with this kind of problem in MicroPython is to have 2 macros, one that takes no variable arguments and one that does (and must take at least one): https://github.com/micropython/micropython/blob/master/py/compile.c#L67-L68

@geky
Copy link
Member

geky commented Nov 30, 2019

@dpgeorge, that is a good way to handle it! It hadn't even crossed my mind to just make a separate macro for single args. I'll have to pocket this for the future.

I still want to consider the printf("debug: " fmt "%s\n", args, "") option above for the ergonomics.

As far as I can tell the only downside is the runtime overhead of parsing another argument. But the logging functions are exactly expected to be performance sensitivity or lightweight. And if the overhead is a concern, the macros can be locally overwritten to use toolchain specific tricks like @hemmick's PR.

Interested in other thoughts though.

@geky geky added the needs minor version new functionality only allowed in minor versions label Dec 1, 2019
@hemmick
Copy link
Contributor Author

hemmick commented Dec 10, 2019

@geky Thanks! Excellent write-up -- I will use your new method

@geky
Copy link
Member

geky commented Jan 27, 2020

Hi @hemmick, did you have a chance to try the LFS_DEBUG indirection? Is it possible to update this PR? If not I can bring it in and make the suggested change in the next release.

@geky geky added v2.2 next minor and removed needs minor version new functionality only allowed in minor versions labels Jan 27, 2020
__VA_ARGS__ are frustrating in C. Even for their main purpose (printf),
they fall short in that they don't have a _portable_ way to have zero
arguments after the format string in a printf call.

Even if we detect compilers and use ##__VA_ARGS__ where available, GCC
emits a warning with -pedantic that is _impossible_ to explicitly
disable.

This commit contains the best solution we can think of. A bit of
indirection that adds a hidden "%s" % "" to the end of the format
string. This solution does not work everywhere as it has a runtime
cost, but it is hopefully ok for debug statements.
@geky geky merged commit 4a9bac4 into littlefs-project:master Mar 31, 2020
@geky
Copy link
Member

geky commented Apr 1, 2020

@hemmick Thanks for the original PR! The improved debug statements should now be available on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants