core: do not use m4 to define PRINTF_FORMAT #1352
Conversation
The PRINTF_FORMAT macro is intended to allow changing how libusb gets warnings about coding warnings for internal logging methods. At this time there is only a single definition and it is always used. It is better to just define it in libusbi.h and not bother with the m4. Signed-off-by: Nathan Hjelm <hjelmn@google.com>
| @@ -100,7 +100,7 @@ extern int ezusb_load_eeprom(libusb_device_handle *device, | |||
| /* Verbosity level (default 1). Can be increased or decreased with options v/q */ | |||
| extern int verbose; | |||
|
|
|||
| extern void logerror(const char *format, ...) PRINTF_FORMAT(1, 2); | |||
| extern void logerror(const char *format, ...); | |||
There was a problem hiding this comment.
I don't think it is actually useful to have it in this particular case. I am not 100% convinced this file should even be including config.h really. Let me run through the code one more time and think this through a bit more. I am not a fan of overloading config.h with things that we shouldn't define using m4. This macro is a good example since the different versions can be determined with a simple #if check.
There was a problem hiding this comment.
I am not 100% convinced this file should even be including config.h really
I'm convinced it shouldn't (logically). A good public example shouldn't use any of the internals sources.
There was a problem hiding this comment.
Removing this takes away the compiler's ability to warn about messing up the format string, and format string screwups can be a source of security issues.
I agree we don't need m4, autotools, cmake or anything else, we just need to create a WeWishCHadThis.h with a bunch of #if / #else to create a PRINTF_FORMAT macro.
| @@ -20,7 +20,8 @@ | |||
| #ifndef LIBUSB_TESTLIB_H | |||
| #define LIBUSB_TESTLIB_H | |||
|
|
|||
| #include <config.h> | |||
| #include "config.h" | |||
| #include "libusbi.h" | |||
There was a problem hiding this comment.
I guess this is the disadvantage of not using m4 (and config.h), now "user" programs need to include this internal header file.
There was a problem hiding this comment.
Well, not exactly a user program so it is not terrible for it to use the internal header. It can't use any of the internal methods but can make use of the defines in there.
There was a problem hiding this comment.
Even if it is defined by the m4 - it is still unusable by the end-user as no one includes these m4 files into their code-bases direcly.
Things like __attribute__/__format__ are not standard/compiler-specific and this is one of the cases where each mature-enough project, where cross-platform support is needed, defines its own set of support/macros.
Right here we're only leveraging the fact that this is still "our" (libusb internal) code and we can share/use other code indended for the "main" code-base.
|
I guess the main thing is I am having second thoughts on the usefulness of: Putting this in so many different places like that doesn't make much sense. libusbi.h seems like the right place but maybe we should have another internal header specifically for this kind of thing. config.h is mean for things that will change at configure time not static code. |
|
I am leaning toward a new header: libusb_compat.h that will hold the two compatibility macros we currently define in config.h. These are |
| @@ -310,6 +310,8 @@ int usbi_vsnprintf(char *dst, size_t size, const char *format, va_list args); | |||
| #define LIBUSB_PRINTF_WIN32 | |||
| #endif /* defined(_MSC_VER) && (_MSC_VER < 1900) */ | |||
|
|
|||
| #define PRINTF_FORMAT(a,b) __attribute__((__format__ (__printf__, a, b))) | |||
There was a problem hiding this comment.
shouldn't it be under ifder/compiler-specific?
I believe this is __GNUC__-specific - MSVC doesn't support it
There was a problem hiding this comment.
and I believe we need an #else fallback as well
|
+1 for not using m4. How about just #defining PRINTF_FORMAT in libusb.h, just like LIBUSB_DEPRECATED_FOR and ZERO_SIZED_ARRAY. |
|
This is still WIP or under discussion, right? Feel free to un-DRAFT it when it seems ready, |
The PRINTF_FORMAT macro is intended to allow changing how libusb gets warnings about coding warnings for internal logging methods. At this time there is only a single definition and it is always used. It is better to just define it in libusbi.h and not bother with the m4.