Elegant bit twiddling?

[poteto]

I don't really want to use <bitset> because I only want to use a unsigned char for the flags, not a ulong (I could cast but that's only for desperation!), sorry for not asking this on a C forum, but I want to believe that I am just doing this wrong, and could be done better. (I looked at bitset for a bit and I find that it doesn't really do anything special other than putting the bit operators into functions). I have a hunch that maybe the bit shifting operator is the key, but my brain just doesn't click...

The all caps words are in a enum with values that are a multiple of 2. IE: 1, 2, 4, 8. log_flags is a char, and it has a few default flags.

Just to make this clear, this code works 100%, but I am not happy with how it looks, and I want to believe it could be shorter (as in more easier to be read).

    //there are default values that I want to respect
    bool l_general = (log_flags & LOG_GENERAL),
        l_event = (log_flags & LOG_EVENT),
        l_actors = (log_flags & LOG_ACTOR),
        l_lua = (log_flags & LOG_LUA),
        l_debug = (log_flags & LOG_DEBUG);

    //these functions could return the result if only 1 argument is given, but in this form if the value is not found the default value is preserved. 
    option_file.find<bool>("Shown_Logs.general", l_general);
    option_file.find<bool>("Shown_Logs.events", l_event);
    option_file.find<bool>("Shown_Logs.actors", l_actors);
    option_file.find<bool>("Shown_Logs.lua", l_lua);
    option_file.find<bool>("Shown_Logs.debug", l_debug);

    //I wish I didn't need ternary operators :S
    log_flags ^= ( l_general != (log_flags & LOG_GENERAL) ? LOG_GENERAL :0 )
              | ( l_event != (log_flags & LOG_EVENT) ? LOG_EVENT :0 )
              | ( l_actors != (log_flags & LOG_ACTOR) ? LOG_ACTOR :0 )
              | ( l_lua != (log_flags & LOG_LUA) ? LOG_LUA :0 )
              | ( l_debug != (log_flags & LOG_DEBUG) ? LOG_DEBUG :0 );

[tpb]

You could get rid of the l_ vars and put the steps into one function that you call like this:

    option_file.set<bool>("Shown_Logs.general", LOG_GENERAL, log_flags);
    option_file.set<bool>("Shown_Logs.events", LOG_EVENT, log_flags);
    ...

// pseudocode
set(const char *label, unsigned flagmask, unsigned &flags):
    if (bool(flags & flagmask) != /*option_file.*/find<bool>(label))
        flags ^= flagmask;

[dhayden]

I'm confused. At line 2 you set l_general = log_flags & LOG_GENERAL. At line 16 you compare l_general to the same value. Isn't the result always true?

[tpb]

Isn't the result always true?

I'm assuming that find<bool> sets l_general, etc., to whatever it finds in the options file but if it doesn't appear in the options file then he wants it to retain it's default value as given in log_flags on input. So the final expression is only using the bitmasks of the values that changed, the others being set to 0.

[poteto]

tpb that solution is exactly what I want, its so simple! By coincidence, option_file is wrapper I wrote, so that snippet is really convenient.

And yea dhayden, tpb explained exactly how it is.

[tpb]

Glad I could help!

[jonnin]

Depending on what you are doing, a bitfield has merits (not here) and you can also use an enum to hold some common bit masks, eg if FlagAndDebug&mybitsint) instead of (if mybitsint &1 && mybitsint&2) or the like. Dunno if that would make it any more 'elegant' or not.

[dhayden]

If I understand this right, you have a default set of bits in log_flag. The defaults can be overridden by the appropriate settings in option_file. If that's right then the solutions so far won't work right because they don't distinguish between "the options file says false" and "the options file has no entry for this keyword."

To handle this, I'd have option_file<>.find() work like this:

// Find an option in the option file. If found then return true and set value to the found value.
// Otherwise return false and leave value unchanged.
bool option_file.find<T>(const char *keyword, T&value);

Then to read the log flags, you can use a short table and a loop:

struct LogTable {
    const char *key;
    unsigned bit;
};
LogTable options[] = {
    {"Shown_Logs.general", LOG_GENERAL },
    {"Shown_Logs.events", LOG_EVENT },
    {"Shown_Logs.actors", LOG_ACTOR },
    {"Shown_Logs.lua", LOG_LUA },
    {"Shown_Logs.debug", LOG_DEBUG }
};

for (LogTable & entry : options) {
    bool value;
    if (option_file.find<bool>(entry.key, value)) {
        if (value) log_flag |= option.bit;
        else log_flags &= ~option.bit;
    }
 }

[tpb]

dhayden is right. I wrecked your code when I translated it. It should be more like this:

    option_file.set<bool>("Shown_Logs.general", LOG_GENERAL, log_flags);
    option_file.set<bool>("Shown_Logs.events", LOG_EVENT, log_flags);
    ...

// pseudocode
set(const char *label, unsigned flagmask, unsigned &flags):
    bool l_flag = flags & flagmask;
    find<bool>(label, l_flag);
    if (bool(flags & flagmask) != l_flag)
        flags ^= flagmask;

[poteto]

You are right, I forgot to test what happens when I remove the option flags, it seems to give the opposite results that my defaults have. I guess I completely ignored why did I use those ternary operators in the first place!

  bool setbit(const std::string& key, unsigned char flagmask, unsigned char &flags)
  {
    bool value;
    if(find<bool>(key,value))
    {
      if(value) flags |= flagmask;
      else flags &= ~flagmask;
      return true;
    }
    return false;
  }

I also played around and got it working with xor too.

  bool setbit(const std::string& key, unsigned char flagmask, unsigned char &flags)
  {
    bool value;
    if(find<bool>(key,value))
    {
      if(bool(flags & flagmask) != value) flags ^= flagmask;
      return true;
    }
  return false;
  }

[poteto]

NINJA'D