apache
THRIFT-3268: Suppress gnu-zero-variadic-macro-arguments warnings
Client: cpp
We can reproduce these warnings by:
CC=clang CXX=clang++ \
cmake \
-S . \
-B ../thrift.build \
-DWITH_{AS3,JAVA,JAVASCRIPT,NODEJS,PYTHON,C_GLIB}=OFF \
-DCMAKE_CXX_FLAGS="-Wgnu-zero-variadic-macro-arguments"
cmake --build ../thrift.build
Sample warning:
lib/cpp/src/thrift/TLogging.h:119:13: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
119 | ##__VA_ARGS__); \
| ^
- [ ] Did you create an Apache Jira ticket? (Request account here, not required for trivial changes)
- It already exists.
- [x] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
- [x] Did you squash your changes to a single commit? (not required, but preferred)
- [x] Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
T_DEBUG("...")is rejected by this change. We need to useT_DEBUG("%s", "...")instead.
- [x] If your change does not involve any code, include
[skip ci]anywhere in the commit message to free up build resources.
I'm not so sure about this one. Introducing a breaking change (per JIRA label) and looking at the considerations behind (e.g. this SO thread to name just one) that looks to me like a simple but effective way to introduce funny issues.
OK. Are you interested in suppressing this warning? If so, I'll look into another approach that doesn't introduce any backward compatibility. (In the implementation, `T_DEBUG("...") is rejected.)
If you aren't interested in suppressing this warning, I close this. (Should we close https://issues.apache.org/jira/browse/THRIFT-3268 too?)
OK. How about using #define XXX(...) + fprintf(stderr, __VA_ARGS__) instead of #define XXX(format_string, ...) + fprintf(stderr, format_string, ##__VA_ARGS__)? It still can reject XXX() (but error message is changed) and accept XXX("...").
diff --git a/lib/cpp/src/thrift/TLogging.h b/lib/cpp/src/thrift/TLogging.h
index 07ff030f7..c4935f757 100644
--- a/lib/cpp/src/thrift/TLogging.h
+++ b/lib/cpp/src/thrift/TLogging.h
@@ -53,9 +53,11 @@
* @param format_string
*/
#if T_GLOBAL_DEBUGGING_LEVEL > 0
-#define T_DEBUG(format_string, ...) \
+#define T_DEBUG(...) \
if (T_GLOBAL_DEBUGGING_LEVEL > 0) { \
- fprintf(stderr, "[%s,%d] " format_string " \n", __FILE__, __LINE__, ##__VA_ARGS__); \
+ fprintf(stderr, "[%s,%d] ", __FILE__, __LINE__); \
+ fprintf(stderr, __VA_ARGS__); \
+ fprintf(stderr, " \n"); \
}
#else
#define T_DEBUG(format_string, ...)
@@ -67,7 +69,7 @@
* @param string format_string input: printf style format string
*/
#if T_GLOBAL_DEBUGGING_LEVEL > 0
-#define T_DEBUG_T(format_string, ...) \
+#define T_DEBUG_T(...) \
{ \
if (T_GLOBAL_DEBUGGING_LEVEL > 0) { \
time_t now; \
@@ -76,11 +78,12 @@
THRIFT_CTIME_R(&now, dbgtime); \
dbgtime[24] = '\0'; \
fprintf(stderr, \
- "[%s,%d] [%s] " format_string " \n", \
+ "[%s,%d] [%s] ", \
__FILE__, \
__LINE__, \
- dbgtime, \
- ##__VA_ARGS__); \
+ dbgtime); \
+ fprintf(stderr, __VA_ARGS__); \
+ fprintf(stderr, " \n"); \
} \
}
#else
@@ -94,9 +97,11 @@
* @param int level: specified debug level
* @param string format_string input: format string
*/
-#define T_DEBUG_L(level, format_string, ...) \
+#define T_DEBUG_L(level, ...) \
if ((level) > 0) { \
- fprintf(stderr, "[%s,%d] " format_string " \n", __FILE__, __LINE__, ##__VA_ARGS__); \
+ fprintf(stderr, "[%s,%d] ", __FILE__, __LINE__); \
+ fprintf(stderr, __VA_ARGS__); \
+ fprintf(stderr, " \n"); \
}
/**
@@ -104,7 +109,7 @@
*
* @param string format_string input: printf style format string
*/
-#define T_ERROR(format_string, ...) \
+#define T_ERROR(...) \
{ \
time_t now; \
char dbgtime[26]; \
@@ -112,11 +117,12 @@
THRIFT_CTIME_R(&now, dbgtime); \
dbgtime[24] = '\0'; \
fprintf(stderr, \
- "[%s,%d] [%s] ERROR: " format_string " \n", \
+ "[%s,%d] [%s] ERROR: ", \
__FILE__, \
__LINE__, \
- dbgtime, \
- ##__VA_ARGS__); \
+ dbgtime); \
+ fprintf(stderr, __VA_ARGS__); \
+ fprintf(stderr, " \n"); \
}
/**
@@ -125,7 +131,7 @@
*
* @param string format_string input: printf style format string
*/
-#define T_ERROR_ABORT(format_string, ...) \
+#define T_ERROR_ABORT(...) \
{ \
time_t now; \
char dbgtime[26]; \
@@ -133,11 +139,12 @@
THRIFT_CTIME_R(&now, dbgtime); \
dbgtime[24] = '\0'; \
fprintf(stderr, \
- "[%s,%d] [%s] ERROR: Going to abort " format_string " \n", \
+ "[%s,%d] [%s] ERROR: Going to abort ", \
__FILE__, \
__LINE__, \
- dbgtime, \
- ##__VA_ARGS__); \
+ dbgtime); \
+ fprintf(stderr, __VA_ARGS__); \
+ fprintf(stderr, " \n"); \
exit(1); \
}
@@ -147,7 +154,7 @@
* @param string format_string input: printf style format string
*/
#if T_GLOBAL_LOGGING_LEVEL > 0
-#define T_LOG_OPER(format_string, ...) \
+#define T_LOG_OPER(...) \
{ \
if (T_GLOBAL_LOGGING_LEVEL > 0) { \
time_t now; \
@@ -155,7 +162,9 @@
time(&now); \
THRIFT_CTIME_R(&now, dbgtime); \
dbgtime[24] = '\0'; \
- fprintf(stderr, "[%s] " format_string " \n", dbgtime, ##__VA_ARGS__); \
+ fprintf(stderr, "[%s] ", dbgtime); \
+ fprintf(stderr, __VA_ARGS__); \
+ fprintf(stderr, " \n"); \
} \
}
#else
OK. Rebased and force pushed.
BTW, which approach do you prefer?
- The current PR content rejects
T_DEBUG("format only")(T_DEBUG("%S", "format only")is alternative). It breaks API compatibility. - https://github.com/apache/thrift/pull/3187#issuecomment-3567154183 doesn't have API incompatibility but build error message is changed.