Bit flagging right?



  • <FONT size=2></FONT><FONT color=#0000ff size=2><FONT color=#0000ff size=2>internal</FONT></FONT><FONT size=2> </FONT><FONT color=#0000ff size=2><FONT color=#0000ff size=2>enum</FONT></FONT><FONT size=2> </FONT><FONT color=#2b91af size=2><FONT color=#2b91af size=2>QLoggingLevel</FONT></FONT><FONT size=2> </FONT></FONT><FONT size=2>

    {

    LOG_NONE = 0,

    LOG_ERRORS = 1,

    LOG_WARNINGS = 2,

    LOG_COMMENTS = 4,

    LOG_METHOD_ENTRY = 8,

    LOG_METHOD_EXIT = 16,

    LOG_SERVER_CALLS = 32,

    LOG_ALL = 127

    };

     

    Ok so everything looks good till we get to the end.  Now I ask why do we need the LOG_ALL entry and why isn't the value 63?  Did someone forget what bitflagging was for.  And yes they do OR them to add logging levels together and then AND them to check if it is set.

    </FONT>


  • In case someone adds a 64 value flag, and wants it included in LOG_ALL?



  • Yeah, TRWTF is that LOG_ALL should be INT_MAX. Future expansion!



  • @pkmnfrk said:

    In case someone adds a 64 value flag, and wants it included in LOG_ALL?

    And if you set logging = LOG_ALL then check for logging at a certian level by anding the value you want to check with the bitflags, you get no logging.

    LOG_All becomes another setting of LOG_NONE

     

    i.e.

    <FONT size=2></FONT><FONT color=#0000ff size=2><FONT color=#0000ff size=2>internal</FONT></FONT><FONT size=2> </FONT><FONT color=#0000ff size=2><FONT color=#0000ff size=2>static</FONT></FONT><FONT size=2> </FONT><FONT color=#0000ff size=2><FONT color=#0000ff size=2>bool</FONT></FONT><FONT size=2> IsLoggingEnabled(</FONT><FONT color=#2b91af size=2><FONT color=#2b91af size=2>QLoggingLevel</FONT></FONT><FONT size=2> level)</FONT></FONT><FONT size=2>

    {

    </FONT><FONT color=#0000ff size=2><FONT color=#0000ff size=2>return</FONT></FONT><FONT size=2> (m_LogLevel & (</FONT><FONT color=#0000ff size=2><FONT color=#0000ff size=2>int</FONT></FONT><FONT size=2>)level) == (</FONT><FONT color=#0000ff size=2><FONT color=#0000ff size=2>int</FONT></FONT><FONT size=2>)level;</FONT></FONT><FONT size=2>

    }

    </FONT>


  • @KattMan said:

    @pkmnfrk said:

    In case someone adds a 64 value flag, and wants it included in LOG_ALL?

    And if you set logging = LOG_ALL then check for logging at a certian level by anding the value you want to check with the bitflags, you get no logging.

    LOG_All becomes another setting of LOG_NONE

     

    i.e.

    <font size="2"></font><font color="#0000ff" size="2"><font color="#0000ff" size="2">internal</font></font><font size="2"> </font><font color="#0000ff" size="2"><font color="#0000ff" size="2">static</font></font><font size="2"> </font><font color="#0000ff" size="2"><font color="#0000ff" size="2">bool</font></font><font size="2"> IsLoggingEnabled(</font><font color="#2b91af" size="2"><font color="#2b91af" size="2">QLoggingLevel</font></font><font size="2"> level)</font><font size="2"> </font>

    <font size="2">{</font>

    <font color="#0000ff" size="2"><font color="#0000ff" size="2">return</font></font><font size="2"> (m_LogLevel & (</font><font color="#0000ff" size="2"><font color="#0000ff" size="2">int</font></font><font size="2">)level) == (</font><font color="#0000ff" size="2"><font color="#0000ff" size="2">int</font></font><font size="2">)level;</font><font size="2">

    }

    </font>
     

     

    How do you figure?  127 & 4 == 4, yes?

     

     



  • @KattMan said:

    @pkmnfrk said:

    In case someone adds a 64 value flag, and wants it included in LOG_ALL?

    And if you set logging = LOG_ALL then check for logging at a certian level by anding the value you want to check with the bitflags, you get no logging.

    LOG_All becomes another setting of LOG_NONE

     

    i.e.

    <font size="2"></font><font color="#0000ff" size="2"><font color="#0000ff" size="2">internal</font></font><font size="2"> </font><font color="#0000ff" size="2"><font color="#0000ff" size="2">static</font></font><font size="2"> </font><font color="#0000ff" size="2"><font color="#0000ff" size="2">bool</font></font><font size="2"> IsLoggingEnabled(</font><font color="#2b91af" size="2"><font color="#2b91af" size="2">QLoggingLevel</font></font><font size="2"> level)</font><font size="2">

    {

    </font><font color="#0000ff" size="2"><font color="#0000ff" size="2">return</font></font><font size="2"> (m_LogLevel & (</font><font color="#0000ff" size="2"><font color="#0000ff" size="2">int</font></font><font size="2">)level) == (</font><font color="#0000ff" size="2"><font color="#0000ff" size="2">int</font></font><font size="2">)level;</font><font size="2">

    }

    </font>

    This is a variation on the == true pattern that seems so pervasive. Why not code it like this:

    internal static bool IsLoggingEnabled(QLoggingLevel level)
    {
    return (m_LogLevel & (int)level) != 0;
    }
    


  • @pkmnfrk said:

    This is a variation on the == true pattern that seems so pervasive. Why not code it like this:

    internal static bool IsLoggingEnabled(QLoggingLevel level)
    {
    return (m_LogLevel & (int)level) != 0;
    }

    You're assuming that "level" is only the result of only one flag being set. Now you can't see if both warnings and errors are set with a single call.

    I don't know why you'd want to do that, but there you go. Maybe other combinations are more interesting.



  • @boomzilla said:

    @pkmnfrk said:

    This is a variation on the == true pattern that seems so pervasive. Why not code it like this:

    internal static bool IsLoggingEnabled(QLoggingLevel level)
    {
    return (m_LogLevel & (int)level) != 0;
    }

    You're assuming that "level" is only the result of only one flag being set. Now you can't see if both warnings and errors are set with a single call.

    I don't know why you'd want to do that, but there you go. Maybe other combinations are more interesting.

     

    That would be the difference between AND and OR, wouldn't it?

    if level is say 6 (LOG_WARNINGS|LOG_COMMENTS) then (psuedocoded) "(mLogLevel & level) == level" would be true  only when a log item is both a comment and warning, while "(mLogLevel & level) != 0" would be true when a log item is either a comment or a warning (or both). The latter would be required for LOG_ALL to work, the former would be false for all possible values.

    (I'm reading the details correctly?)

     



  • Here's the idea though, upi have a logging mask that sets bits for the type of logging you want.

    So I want Errors AND Warnings AND comments so those flags get set.

    Then during logging the code can simply say If(IsLoggingEnabled(ERRORS)) it will log errors then ask IF(IsLoggingEnabled(WARNINGS) then log warnings.  No special code needed but you are checking for each different one each time you try to log.

    thing is there is no reason for a LOG_ALL because you aren't ever checking for LOG_ALL, you check for the type of log entry you are writing, you also never add LOG_ALL, you instead AND the ones together you want so you get the mask of what is wanted by all parts of the logic flow, somtimes dependant on config settings, etc.

    So we should AND to set, and OR to check, always.



  •  So how would you do setLogging( QLoggingLevel )?



  • You might grok the code base for usages of LOG_ALL to see if it is being (mis)used anywhere.  Who knows but that you might learn something "useful" (if not for personal enlightenment, for the entertainment of your fellows in these parts).

    @Severity One said:

     So how would you do setLogging( QLoggingLevel )?

    You don't call setLogging(QLoggingLevel), you call setLogging(int), and you pass in the combination of logging levels or'd together. Of course that should only be done on app startup by the log configurator after reading in the logging configuration from file. Similarly the log level check should be wrapped within the corresponding log.error/log.warn/log.info/etc. method call to make things simple for the people who are using this framework (and to make it easy to switch to a more standard logging framework when they find out that that's all they're rewriting anyway).



  • @airdrik said:

    You might grok the code base for usages of LOG_ALL to see if it is being (mis)used anywhere.  Who knows but that you might learn something "useful" (if not for personal enlightenment, for the entertainment of your fellows in these parts).

    @Severity One said:

     So how would you do setLogging( QLoggingLevel )?

    You don't call setLogging(QLoggingLevel), you call setLogging(int), and you pass in the combination of logging levels or'd together. Of course that should only be done on app startup by the log configurator after reading in the logging configuration from file. Similarly the log level check should be wrapped within the corresponding log.error/log.warn/log.info/etc. method call to make things simple for the people who are using this framework (and to make it easy to switch to a more standard logging framework when they find out that that's all they're rewriting anyway).



    So if you want everything logged, you'd do

    setLogging(LOG_ERRORS | LOG_WARNINGS <font size="2">| </font>LOG_COMMENTS <font size="2">| </font>LOG_METHOD_ENTRY <font size="2">| </font>LOG_METHOD_EXIT<font size="2"> | </font>LOG_SERVER_CALLS);

     But what if another log type is added later? Then you'd have to find every instance of setLogging(...) and add the new log type. Wouldn't it be cool to have aconstant like LOG_ALL, which is defined right there together with the other constants, so you won't forget to change it?

     LOG_ALL could also be used to enumerate all logging types, by iterating over all bits of that constant.

     Also, the constant may be useful during debugging. You've got the applications halted in a debugger and you need logging right now? Just enable LOG_ALL and don't worry about what types to enable.

     



  • @geocities said:

    stuff snipped

    Tag Troll



  • There are some issues but I'm not sure they are WTF-level.

    If somebody wants to see if ANY logging is enabled, and does so by checking logLevel & LOG_ALL != 0, this will return true incorrectly if some garbage bit is set (i.e. the 64 bit, which isn't defined to have any meaning)

    Chances are that in the past there was a LOG_XXX = 64, which was removed and LOG_ALL was not updated.

    Despite the potential problem, I have seen this commonly and don't remember any particular issues caused by such a thing.



  • This reminds me of something I saw the other day in PHP:

    error_reporting(E_ALL || !E_NOTICE);

    D:



  • @morbiuswilters said:

    This reminds me of something I saw the other day in PHP:

    error_reporting(E_ALL || !E_NOTICE);

    D:

    I see a few ways that snippet is outright wrong, a way that it would be pointless if the errors were corrected, and TRWTF that is PHP (where the expression E_ALL & E_STRICT === 0).



  • @pkmnfrk said:

    I see a few ways that snippet is outright wrong

    A few ways? There's one way.

    @pkmnfrk said:

    a way that it would be pointless if the errors were corrected

    Explain.

    @pkmnfrk said:

    and TRWTF that is PHP (where the expression E_ALL & E_STRICT === 0)

    I basically don't think E_STRICT or E_NOTICE should even exist. Regardless, as of 5.4.0 E_STRICT is a part of E_ALL.


Log in to reply
 

Looks like your connection to What the Daily WTF? was lost, please wait while we try to reconnect.