Rewrite wxLogXXX() macros to avoid "ambiguous else" warnings.

Use a dummy for loop instead of an if statement to avoid all problems with the
dangling else clauses: both the need for an artificially inversed "if" to make
the code like

	if ( something )
		wxLogError("...");
	else
		something-else;

to work as expected and to avoid warnings given by some versions of g++ and
clang for the code above advising to add explicit braces.

Closes #11829.

git-svn-id: https://svn.wxwidgets.org/svn/wx/wxWidgets/trunk@74735 c3d73ce0-8a6f-49c7-b76d-6d57e0e08775

diff --git a/include/wx/log.h b/include/wx/log.h
index 6e29d7d77d32d5320fe50885fe9e9a37f81fde0d..fbe855b00987410e3f42c754d49feb39fa1f6f16 100644 (file)
--- a/include/wx/log.h
+++ b/include/wx/log.h
@@ -12,6 +12,7 @@
 #define _WX_LOG_H_
 
 #include "wx/defs.h"
 #define _WX_LOG_H_
 
 #include "wx/defs.h"

+#include "wx/cpp.h"

 
 // ----------------------------------------------------------------------------
 // types
 
 // ----------------------------------------------------------------------------
 // types


@@ -1329,29 +1330,27 @@ WXDLLIMPEXP_BASE const wxChar* wxSysErrorMsg(unsigned long nErrCode = 0);
 // following arguments are not even evaluated which is good as it avoids
 // unnecessary overhead)
 //
 // following arguments are not even evaluated which is good as it avoids
 // unnecessary overhead)
 //

-// Note: the strange if/else construct is needed to make the following code
+// 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
 //              ...
 //
 //
 //          if ( cond )
 //              wxLogError("!!!");
 //          else
 //              ...
 //

-//       work as expected, without it the second "else" would match the "if"
-//       inside wxLogError(). Unfortunately code like
-//
-//          if ( cond )
-//              wxLogError("!!!");
-//
-//       now provokes "suggest explicit braces to avoid ambiguous 'else'"
-//       warnings from g++ 4.3 and later with -Wparentheses on but they can be
-//       easily fixed by adding curly braces around wxLogError() and at least
-//       the code still does do the right thing.
-#define wxDO_LOG_IF_ENABLED(level)                                            \
-    if ( !wxLog::IsLevelEnabled(wxLOG_##level, wxLOG_COMPONENT) )             \
-    {}                                                                        \
-    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)
 
         wxDO_LOG(level)
 

+#define wxDO_LOG_IF_ENABLED(level)                                            \
+    wxDO_LOG_IF_ENABLED_HELPER(level, wxMAKE_UNIQUE_NAME(wxlogcheck))
+

 // wxLogFatalError() is special as it can't be disabled
 #define wxLogFatalError wxDO_LOG(FatalError)
 #define wxVLogFatalError(format, argptr) wxDO_LOGV(FatalError, format, argptr)
 // wxLogFatalError() is special as it can't be disabled
 #define wxLogFatalError wxDO_LOG(FatalError)
 #define wxVLogFatalError(format, argptr) wxDO_LOGV(FatalError, format, argptr)

diff --git a/tests/log/logtest.cpp b/tests/log/logtest.cpp
index 5edbb84e3049eeabb830488b95d7c54cc6123383..55988504a15714e359023df70e143e8d3570ecdb 100644 (file)
--- a/tests/log/logtest.cpp
+++ b/tests/log/logtest.cpp
@@ -169,6 +169,7 @@ private:
         CPPUNIT_TEST( CompatLogger2 );
 #endif // WXWIN_COMPATIBILITY_2_8
         CPPUNIT_TEST( SysError );
         CPPUNIT_TEST( CompatLogger2 );
 #endif // WXWIN_COMPATIBILITY_2_8
         CPPUNIT_TEST( SysError );

+        CPPUNIT_TEST( NoWarnings );

     CPPUNIT_TEST_SUITE_END();
 
     void Functions();
     CPPUNIT_TEST_SUITE_END();
 
     void Functions();


@@ -182,6 +183,7 @@ private:
     void CompatLogger2();
 #endif // WXWIN_COMPATIBILITY_2_8
     void SysError();
     void CompatLogger2();
 #endif // WXWIN_COMPATIBILITY_2_8
     void SysError();

+    void NoWarnings();

 
     TestLog *m_log;
     wxLog *m_logOld;
 
     TestLog *m_log;
     wxLog *m_logOld;


@@ -362,3 +364,23 @@ void LogTestCase::SysError()
 #endif // __MINGW32__
 }
 
 #endif // __MINGW32__
 }
 

+void LogTestCase::NoWarnings()
+{
+    // Check that "else" branch is [not] taken as expected and that this code
+    // compiles without warnings (which used to not be the case).
+
+    bool b = wxFalse;
+    if ( b )
+        wxLogError("Not logged");
+    else
+        b = !b;
+
+    CPPUNIT_ASSERT( b );
+
+    if ( b )
+        wxLogError("If");
+    else
+        CPPUNIT_FAIL("Should not be taken");
+
+    CPPUNIT_ASSERT_EQUAL( "If", m_log->GetLog(wxLOG_Error) );
+}