Added FlagSet utility class

[peci1]

This class helps managing a set of flags that are defined as a C++11 enum class.

Adapted from https://github.com/mrts/flag-set-cpp (MIT license, compatible with Apache 2.0).

I have a use for this class in my next PR (not yet published as it needs polishing). But I think it might be generally useful to have a canonical way to work with flag sets.

[mjcarroll]

This is a bit difficult to evaluate without more context, do you think you could open a draft PR showing the intended use?

[peci1]

Okay, here's the example usage:

https://github.com/ignitionrobotics/ign-common/blob/2367c1cb1ae78b4b517efbdbdfa85bc6d525c10f/av/include/ignition/common/HWVideo.hh#L23-L38

https://github.com/ignitionrobotics/ign-common/blob/2367c1cb1ae78b4b517efbdbdfa85bc6d525c10f/av/src/VideoEncoder.cc#L165-L192

[mjcarroll]

One piece of API feedback in comments.

Does it also make sense to implement std::hash the same way that bitset does so that it is usable in things like unordered_map and unordered_set?

[peci1]

Do you have a use-case for indexing by bitsets? I can imagine indexing by enum values, but by whole bitsets?

[mjcarroll]

Do you have a use-case for indexing by bitsets? I can imagine indexing by enum values, but by whole bitsets?

I was mainly thinking of parity with the bitset API, but off hand, here are two things I could think of:

Dispatching based on a certain configuration of flags, so maybe:

std::unorderd_map<FlagSet, std::function<void(void)>> dispatch = {
  {FlagSet::AllEnabled, doFoo},
  {FlagSet::AllDisabled, doBar},
};

Or potentially keeping a set of valid configurations to check against:

std::set<FlagSet> valid;
valid.contains(FlagSet::AllEnabled);

[mjcarroll]

@peci1 friendly ping

[peci1]

I'll have a look at it over Xmas. This week's my vacation.

[chapulina]

@peci1 , do you need this to go into ign-common3 so it's available to released collections (Citadel and Dome)? If not, we're starting a new package to hold this kind of utility that doesn't bring any dependencies: https://github.com/ignitionrobotics/ign-utils/. That will be released into Edifice, but it's not clear yet what downstream libraries will depend on it.

[chapulina]

do you need this to go into ign-common3

Oh I just saw that you're using it for the video encoder in #125 , so that answers my question.

[peci1]

Yes, you're right, I'd like to use this PR in the hardware support for video encoder. But it could be possible to factor this out to a new package starting with Edifice.

[peci1]

I did what was requested (renamed the static constructors and added support for std::hash).

I got one more idea - now the class requires that the last element of the enum is an underscore. That is because of two things:

1. determine the size of the bitset in private: std::bitset<static_cast<UnderlyingType>(T::_)> flags;
2. disable FlagSet operator|(const T& _lhs, const T& _rhs) for enums (or other types) not containing the underscore (as a safety measure, otherwise all types would be OReable to a FlagSet)

I think 1. can be overcome by exposing an explicit template argument for the number of elements in the enum. That would allow much broader use of FlagSet even with enums you do not control. You would just have to pass the correct number of elements. I know this can be difficult to maintain, but many projects use something other than underscore, e.g. END or other values to signal the last enum element.

It seems that it is not possible to overcome 2., so let's say only underscore-ending enums would get this syntactic sugar (ORing two base values creates the FlagSet containing both of them). But that's just syntactic sugar.

What do you think about this?

[mjcarroll]

I don't think I mind having the underscore as the last entry in this case. That seems easier (maintenance-wise) than having to keep track of the number of elements. I do see the advantage of exposing the other alternative in the case that it is an enum that we do not maintain/control.

I also agree that disabling the | for things without the underscore is reasonable, and that functionality is reserved for enums with the correct sentinel value.

[mjcarroll]

Friendly ping @peci1, is there anything else you want to do here?

[peci1]

I implemented the option for explicit specification of the last element (and whether to treat it as a valid member of the enum or not). I also added some more convenience methods like initializer-list constructor.

I'm satisfied with this PR now (but feel free to comment on the new changes).