Log4cxx
  1. Log4cxx
  2. LOGCXX-288

Logging statement requires braces around it in an if-else

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.10.0
    • Fix Version/s: 0.11.0
    • Component/s: None
    • Labels:
      None
    • Environment:
      All

      Description

      In converting some code to Log4CXX ended up with an if-else like this:

      if(bMiniDumpSuccessful == TRUE)
      LOG4CXX_FATAL( mainLogger, "Core Dump Successful!" );
      else
      ...

      This won't compile unless you either surround the logging statement with braces or leave off the semicolon. The above code essentially produces something like this:

      if(bMiniDumpSuccessful == TRUE)
      {
      if (mainLogger->isFatalEnabled())

      { .... }

      }
      ;
      else

      And it rightly complains that the else has no if associated with it.

        Activity

        Dale King created issue -
        Hide
        Curt Arnold added a comment -

        The current macros are consistent with log4cxx 0.9.7 which did not require trailing semi-colons. If the macros were changed to require semi-colons (which would allow the reported code to compile), some code that had compiled with log4cxx 0.9.7 would now break.

        An acceptable fix is to eliminate the use of the unnecessary trailing semi's in the examples and tests. Code that did follow the macro evaluation with a semi would compile except in cases similar to what was described in the original bug report, though it may result in warnings with the Sun compilers (see LOGCXX-266).

        Committed rev 668828.

        Show
        Curt Arnold added a comment - The current macros are consistent with log4cxx 0.9.7 which did not require trailing semi-colons. If the macros were changed to require semi-colons (which would allow the reported code to compile), some code that had compiled with log4cxx 0.9.7 would now break. An acceptable fix is to eliminate the use of the unnecessary trailing semi's in the examples and tests. Code that did follow the macro evaluation with a semi would compile except in cases similar to what was described in the original bug report, though it may result in warnings with the Sun compilers (see LOGCXX-266 ). Committed rev 668828.
        Curt Arnold made changes -
        Field Original Value New Value
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Fix Version/s 0.10.1 [ 12313090 ]
        Hide
        Andrey added a comment - - edited

        From the logic view the solution of this problem is incorrect.
        Lets describe my opinion in details:

        • 0.9.7 (very very old) contains bug with LOG4CXX_XXX macros (extra ; ) which forced end users write code like:
          if () { LOG4CXX_XXX(); }
          else

          - I beleive less than 0.1% end users write code like:
          if ()
          LOG4CXX_XXX()
          else

          - now log4cxx developers propose always write lo4cxx macros in form:
          LOG4CXX_XXX(...)
          LOG4CXX_XXX(...)
          - I believe 99.99% of end users never will write in such style (looks bad, actually this style is an eyesore to code developers)
          - so now all is the same as it was before. users still will write code like:
          LOG4CXX_XXX(...);
          LOG4CXX_XXX(...);
          if (){ LOG4CXX_XXX(); }

          else
          And each case when users will write code like above they will remember log4cxx developers (or them curve hands).

        I also know two companies where developers fix these macros.

        I suggest to fix LOG4CXX_XXX() macros. 99.99% of end users will be happy with this enhancement. 0.01% of end user will be required to update code which is not actually terrible.

        Fix is very easy:
        #define LOG4CXX_ERROR(logger, message) do { \
        if (logger->isErrorEnabled()) {\
        ::log4cxx::helpers::MessageBuffer oss_; \
        logger->forcedLog(::log4cxx::Level::getError(), oss_.str(oss_ << message), LOG4CXX_LOCATION); } } while(false)

        Show
        Andrey added a comment - - edited From the logic view the solution of this problem is incorrect. Lets describe my opinion in details: 0.9.7 (very very old) contains bug with LOG4CXX_XXX macros (extra ; ) which forced end users write code like: if () { LOG4CXX_XXX(); } else - I beleive less than 0.1% end users write code like: if () LOG4CXX_XXX() else - now log4cxx developers propose always write lo4cxx macros in form: LOG4CXX_XXX(...) LOG4CXX_XXX(...) - I believe 99.99% of end users never will write in such style (looks bad, actually this style is an eyesore to code developers) - so now all is the same as it was before. users still will write code like: LOG4CXX_XXX(...); LOG4CXX_XXX(...); if (){ LOG4CXX_XXX(); } else And each case when users will write code like above they will remember log4cxx developers (or them curve hands). I also know two companies where developers fix these macros. I suggest to fix LOG4CXX_XXX() macros. 99.99% of end users will be happy with this enhancement. 0.01% of end user will be required to update code which is not actually terrible. Fix is very easy: #define LOG4CXX_ERROR(logger, message) do { \ if (logger->isErrorEnabled()) {\ ::log4cxx::helpers::MessageBuffer oss_; \ logger->forcedLog(::log4cxx::Level::getError(), oss_.str(oss_ << message), LOG4CXX_LOCATION); } } while(false)

          People

          • Assignee:
            Curt Arnold
            Reporter:
            Dale King
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development