Merge branch 'log-dangling-else-warns'

Fix dangling else warnings in wxLog macros.

See https://github.com/wxWidgets/wxWidgets/pull/1755

include/wx/cpp.h

@@ -96,6 +96,27 @@
 #define wxSTATEMENT_MACRO_BEGIN  do {
 #define wxSTATEMENT_MACRO_END } while ( (void)0, 0 )
 
+/*
+    Helper for executing the following statement conditionally without using
+    conditional statements.
+
+    This strange macro is needed in the first place to avoid the problems due
+    to nested if/else inside macros. E.g. if some MACRO started with "if", then
+
+        if ( cond )
+            MACRO();
+        else
+            ...
+
+    would be broken because "..." would bind to the wrong "if" inside the macro
+    rather than the visible one. So we use wxDO_IF() inside the macro instead
+    to avoid this problem.
+ */
+#define wxDO_IF_HELPER(loopvar, condition)                                    \
+    for ( bool loopvar = false; !loopvar && condition; loopvar = true )
+
+#define wxDO_IF(condition) wxDO_IF_HELPER(wxMAKE_UNIQUE_NAME(wxdoif), condition)
+
 /*
     Define __WXFUNCTION__ which is like standard __FUNCTION__ but defined as
     NULL for the compilers which don't support the latter.

include/wx/log.h

@@ -1241,37 +1241,30 @@ WXDLLIMPEXP_BASE wxString wxSysErrorMsgStr(unsigned long nErrCode = 0);
 
 // this macro generates the expression which logs whatever follows it in
 // parentheses at the level specified as argument
-#define wxDO_LOG(level) wxMAKE_LOGGER(level).Log
+#define wxDO_LOG(level) wxDO_LOG_WITH_FUNC(level, Log)
+
+// generalization of the macro above that uses the given function of wxLogger
+// object rather than the default "Log"
+#define wxDO_LOG_WITH_FUNC(level, func) wxMAKE_LOGGER(level).func
 
 // this is the non-vararg equivalent
 #define wxDO_LOGV(level, format, argptr) \
     wxMAKE_LOGGER(level).LogV(format, argptr)
 
-// this macro declares wxLog<level>() macro which logs whatever follows it if
+// Macro evaluating to true if logging at the given level is enabled.
-// logging at specified level is enabled (notice that if it is false, the
+#define wxLOG_IS_ENABLED(level) \
-// following arguments are not even evaluated which is good as it avoids
+    wxLog::IsLevelEnabled(wxLOG_##level, wxLOG_COMPONENT)
-// unnecessary overhead)
-//
-// Note: the strange (because executing at most once) for() loop because we
-//       must arrange for wxDO_LOG() to be at the end of the macro and using a
-//       more natural "if (IsLevelEnabled()) wxDO_LOG()" would result in wrong
-//       behaviour for the following code ("else" would bind to the wrong "if"):
-//
-//          if ( cond )
-//              wxLogError("!!!");
-//          else
-//              ...
-//
-//       See also #11829 for the problems with other simpler approaches,
-//       notably the need for two macros due to buggy __LINE__ in MSVC.
-#define wxDO_LOG_IF_ENABLED_HELPER(level, loopvar)                            \
-    for ( bool loopvar = false;                                               \
-          !loopvar && wxLog::IsLevelEnabled(wxLOG_##level, wxLOG_COMPONENT);  \
-          loopvar = true )                                                    \
-        wxDO_LOG(level)
 
+// Macro used to define most of the actual wxLogXXX() macros: just calls
+// wxLogger::Log(), if logging at the specified level is enabled.
 #define wxDO_LOG_IF_ENABLED(level)                                            \
-    wxDO_LOG_IF_ENABLED_HELPER(level, wxMAKE_UNIQUE_NAME(wxlogcheck))
+    wxDO_IF(wxLOG_IS_ENABLED(level))                                          \
+    wxDO_LOG(level)
+
+// Similar to above, but calls the given function instead of Log().
+#define wxDO_LOG_IF_ENABLED_WITH_FUNC(level, func)                            \
+    wxDO_IF(wxLOG_IS_ENABLED(level))                                          \
+    wxDO_LOG_WITH_FUNC(level, func)
 
 // wxLogFatalError() is special as it can't be disabled
 #define wxLogFatalError wxDO_LOG(FatalError)
@@ -1292,17 +1285,12 @@ WXDLLIMPEXP_BASE wxString wxSysErrorMsgStr(unsigned long nErrCode = 0);
 
 // this one is special as it only logs if we're in verbose mode
 #define wxLogVerbose                                                          \
-    if ( !(wxLog::IsLevelEnabled(wxLOG_Info, wxLOG_COMPONENT) &&              \
+    wxDO_IF(wxLOG_IS_ENABLED(Info) && wxLog::GetVerbose())                    \
-            wxLog::GetVerbose()) )                                            \
+    wxDO_LOG(Info)
-    {}                                                                        \
+
-    else                                                                      \
-        wxDO_LOG(Info)
 #define wxVLogVerbose(format, argptr)                                         \
-    if ( !(wxLog::IsLevelEnabled(wxLOG_Info, wxLOG_COMPONENT) &&              \
+    wxDO_IF(wxLOG_IS_ENABLED(Info) && wxLog::GetVerbose())                    \
-            wxLog::GetVerbose()) )                                            \
+    wxDO_LOGV(Info, format, argptr)
-    {}                                                                        \
-    else                                                                      \
-        wxDO_LOGV(Info, format, argptr)
 
 // another special case: the level is passed as first argument of the function
 // and so is not available to the macro
@@ -1311,10 +1299,8 @@ WXDLLIMPEXP_BASE wxString wxSysErrorMsgStr(unsigned long nErrCode = 0);
 // always evaluated, unlike for the other log functions
 #define wxLogGeneric wxMAKE_LOGGER(Max).LogAtLevel
 #define wxVLogGeneric(level, format, argptr) \
-    if ( !wxLog::IsLevelEnabled(wxLOG_##level, wxLOG_COMPONENT) )             \
+    wxDO_IF(wxLOG_IS_ENABLED(level))                                          \
-    {}                                                                        \
+    wxDO_LOGV(level, format, argptr)
-    else                                                                      \
-        wxDO_LOGV(level, format, argptr)
 
 
 // wxLogSysError() needs to stash the error code value in the log record info
@@ -1331,12 +1317,9 @@ WXDLLIMPEXP_BASE wxString wxSysErrorMsgStr(unsigned long nErrCode = 0);
 // change even much sooner)
 #define wxLOG_KEY_SYS_ERROR_CODE "wx.sys_error"
 
-#define wxLogSysError                                                         \
+#define wxLogSysError \
-    if ( !wxLog::IsLevelEnabled(wxLOG_Error, wxLOG_COMPONENT) )               \
+    wxDO_LOG_IF_ENABLED_WITH_FUNC(Error, MaybeStore(wxLOG_KEY_SYS_ERROR_CODE, \
-    {}                                                                        \
+                                                    wxSysErrorCode()).Log)
-    else                                                                      \
-        wxMAKE_LOGGER(Error).MaybeStore(wxLOG_KEY_SYS_ERROR_CODE,             \
-                                        wxSysErrorCode()).Log
 
 // unfortunately we can't have overloaded macros so we can't define versions
 // both with and without error code argument and have to rely on LogV()
@@ -1350,11 +1333,8 @@ WXDLLIMPEXP_BASE wxString wxSysErrorMsgStr(unsigned long nErrCode = 0);
     // specify the frame to which the message should go
     #define wxLOG_KEY_FRAME "wx.frame"
 
-    #define wxLogStatus                                                       \
+    #define wxLogStatus \
-        if ( !wxLog::IsLevelEnabled(wxLOG_Status, wxLOG_COMPONENT) )          \
+        wxDO_LOG_IF_ENABLED_WITH_FUNC(Status, MaybeStore(wxLOG_KEY_FRAME).Log)
-        {}                                                                    \
-        else                                                                  \
-            wxMAKE_LOGGER(Status).MaybeStore(wxLOG_KEY_FRAME).Log
 
     #define wxVLogStatus \
         wxMAKE_LOGGER(Status).MaybeStore(wxLOG_KEY_FRAME).LogV
@@ -1451,16 +1431,8 @@ public:
 #endif // wxUSE_LOG_DEBUG/!wxUSE_LOG_DEBUG
 
 #if wxUSE_LOG_TRACE
-    #define wxLogTrace                                                        \
+    #define wxLogTrace wxDO_LOG_IF_ENABLED_WITH_FUNC(Trace, LogTrace)
-        if ( !wxLog::IsLevelEnabled(wxLOG_Trace, wxLOG_COMPONENT) )           \
+    #define wxVLogTrace wxDO_LOG_IF_ENABLED_WITH_FUNC(Trace, LogVTrace)
-        {}                                                                    \
-        else                                                                  \
-            wxMAKE_LOGGER(Trace).LogTrace
-    #define wxVLogTrace                                                       \
-        if ( !wxLog::IsLevelEnabled(wxLOG_Trace, wxLOG_COMPONENT) )           \
-        {}                                                                    \
-        else                                                                  \
-            wxMAKE_LOGGER(Trace).LogVTrace
 #else  // !wxUSE_LOG_TRACE
     #define wxVLogTrace(mask, fmt, valist) wxLogNop()
 

tests/log/logtest.cpp

@@ -394,4 +394,67 @@ void LogTestCase::NoWarnings()
     CPPUNIT_ASSERT_EQUAL( "If", m_log->GetLog(wxLOG_Error) );
 }
 
+// The following two functions (v, macroCompilabilityTest) are not run by
+// any test, and their purpose is merely to guarantee that the wx(V)LogXXX
+// macros compile without 'dangling else' warnings.
+#if defined(__clang__) || wxCHECK_GCC_VERSION(4, 6)
+    // gcc 7 split -Wdangling-else from the much older -Wparentheses, so use
+    // the new warning if it's available or the old one otherwise.
+    #if wxCHECK_GCC_VERSION(7, 0)
+        #pragma GCC diagnostic error "-Wdangling-else"
+    #else
+        #pragma GCC diagnostic error "-Wparentheses"
+    #endif
+#endif
+
+static void v(int x, ...)
+{
+    va_list list;
+    va_start(list, x);
+
+    if (true)
+        wxVLogGeneric(Info, "vhello generic %d", list);
+    if (true)
+        wxVLogTrace(wxTRACE_Messages, "vhello trace %d", list);
+    if (true)
+        wxVLogError("vhello error %d", list);
+    if (true)
+        wxVLogMessage("vhello message %d", list);
+    if (true)
+        wxVLogVerbose("vhello verbose %d", list);
+    if (true)
+        wxVLogWarning("vhello warning %d", list);
+    if (true)
+        wxVLogFatalError("vhello fatal %d", list);
+    if (true)
+        wxVLogSysError("vhello syserror %d", list);
+    if (true)
+        wxVLogDebug("vhello debug %d", list);
+
+    va_end(list);
+}
+
+void macroCompilabilityTest()
+{
+    v(123, 456);
+    if (true)
+        wxLogGeneric(wxLOG_Info, "hello generic %d", 42);
+    if (true)
+        wxLogTrace(wxTRACE_Messages, "hello trace %d", 42);
+    if (true)
+        wxLogError("hello error %d", 42);
+    if (true)
+        wxLogMessage("hello message %d", 42);
+    if (true)
+        wxLogVerbose("hello verbose %d", 42);
+    if (true)
+        wxLogWarning("hello warning %d", 42);
+    if (true)
+        wxLogFatalError("hello fatal %d", 42);
+    if (true)
+        wxLogSysError("hello syserror %d", 42);
+    if (true)
+        wxLogDebug("hello debug %d", 42);
+}
+
 #endif // wxUSE_LOG