Add enum class flag definition to platform

[pea-pod]

Summary of changes

Add a macro for adding bitwise operators to enum class types.

This addition is necessary to use bitwise operators on enum class (scoped enums) introduced in C++11.

Without this macro, either the whole boilerplate would have to be written to take advantage of C++ namespaces and scope, or a C style, visible-to-the-world enum would need to be used.

This:

enum class MyEnum {
    HasNose = (1 << 0),
    HasEars = (1 << 1),
    HasFur  = (1 << 2)
};

inline constexpr MyEnum operator |( T lhs, T rhs)
{
    return (MyEnum) ( 
        static_cast<std::underlying_type<MyEnum>::type>(lhs) |
        static_cast<std::underlying_type<MyEnum>::type>(rhs));
}

inline constexpr MyEnum operator &( T lhs, T rhs)
{
    return (MyEnum) ( 
        static_cast<std::underlying_type<MyEnum>::type>(lhs) &
        static_cast<std::underlying_type<MyEnum>::type>(rhs));
}
...
// Repeat for ~, ^=, |= overloads
...
inline MyEnum& operator &= (T & a, T b) { \
    a = a & b; \
    return a; \
}

becomes

MBED_SCOPED_ENUM_FLAGS(MyEnum)  {
    HasNose = (1 << 0),
    HasEars = (1 << 1),
    HasFur  = (1 << 2)
};

Now, using the enum class allows for scope and bitwise operation:

MyEnum cat = MyEnum::HasNose | MyEnum::HasEars | MyEnum::HasFur;
MyEnum neighbor = MyEnum::HasNose | MyEnum::HasEars;

Impact of changes

The only impact is if the macro is used. If the macro is unused, standard enum class definitions are unaffected by the bitwise operators. If the macro is used, the implementing code becomes more concise (and hopefully readable), and the DRY principle is realized.

Regarding code size, the operations should be no different than if written out with static_cast calls.

In the future, standard C enum bit flags could be converted over to enum class for better scoping and collision avoidance.

Migration actions required

If a scoped enum bit flag is desired, include the header and use. Otherwise, none.

Documentation

Documentation including examples are in the file itself.

---

Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[X] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

---

Test results

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

---

Reviewers

Per @0xc0170's request in #12759, this was created as a seperate PR. Also @kjbracey-arm, @VeijoPesonen, and @SeppoTakalo commented, and may be interested as well.

---

[pea-pod]

Some questions I had while writing this PR out:

1. Should I change the filename to mbed_scoped_enum_flags.h or similar?
2. Should I add the shift and shift assignment operators?
3. Should this become a template party instead of a macro fest? (I would need help on this one for sure.)
4. Do I need #if !defined(DOXYGEN_ONLY) around the macros that define the specific overloads (ENUM_FLAG_OPERATOR and ENUM_FLAG_OPERATOR_LHS_REFERENCE)?
5. Should I use MBED_FORCE_INLINE for both of the macros instead of inline?
6. @kjbracey-arm suggested a contextual conversion to bool. I can try to do this, but I am not sure if it is possible, or if it is, that I understand enough to do it.
7. Does this need a test case? If so, I might be out of my league to know where to put it, or how to structure it, etc.
8. Do I need extra Doxygen tags peppering the macro definition, like class or group?

[ciarmcom]

@pea-pod, thank you for your changes.
@VeijoPesonen @SeppoTakalo @kjbracey-arm @ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

[kjbracey]

Thanks for tackling this. If we're going to start having something this, I'd like it to be as good as possible, and if there is such a solution already existing, we may as well pick it up, license permitting.

From the other PR, I found this implementation, which has a number of extra benefits. Given enum class my_bitmask { flag = 1 }; my_bitmask x;:

• if (x & flag) or if (!(x & flag)) works - more concise and natural testing for the most common case
• if (x == flag) doesn't compile. Catches common error. (If you really meant to say that, you can say if (x == my_bitmask(flag)).
• Bool conversion only works on operator results like (x & flag). But if (x) or if (flag) alone doesn't compile. Catches other errors I've seen. (If you really meant to say the first, you can say if (x != my_bitmask()).

The patterns used there also mirror what the most recent updates to the standard C++ library do, in particular the "opt-in" mechanism for user types via the template specialization of enable_xxx<T>.

Now, that code is BSD licensed, so any objection to just adopting it? @bulislaw, @0xc0170? The only thing I'd think I'd do to it is name/namespace adjustments. Something like:

• ::enable_enum_class_bitmask -> mbed::EnableEnumClassBitmask
• enableEnumClassBitmask -> MBED_ENABLE_ENUM_CLASS_BITMASK
• ::bitmask -> mbed::impl::Bitmask (it's private)
• ::enumerator -> mbed::impl::Enumerator (it's private)

The usage model for that would be:

class enum MyEnum  {
    HasNose = (1 << 0),
    HasEars = (1 << 1),
    HasFur  = (1 << 2)
};

MBED_ENABLE_ENUM_CLASS_BITMASK(MyEnum);

Main annoyance of that enable form there is that the "enable" has to be placed at top level, not in any namespace your enum is defined in. But if you get that wrong, it is a compile error, not silent.

On the other hand, separate enabling gives you the ability to specify the underlying type manually with standard syntax (enum class MyEnum : uint8_t), or even to add the operators locally for someone else's enum class where they failed to provide any helpers at all.

Some general answers to a subset of your questions:

\1. For C++ stuff, if's CamelCase. Could be any of EnumBitmask.h, EnumClassBitmask.h, ScopedEnumBitmask.h. No strong preference, but match whatever the code itself uses.
\2. No, no shift operators. If you're shifting the bits, you're losing type-safety on the bit enumeration, Just &, | ,^,~ that maintain bit independence. Anything needing shifts would want some other helper.
\5. inline is fine.
\6. bool at the simplest would just be adding explicit operator bool. if and similar contexts will use an explicit bool conversion. The wrinkle is that ideally you only want that on & results, not just if (cat) or if (MyEnum::HasEars), and that BSD version gives you that.
\8. We'd want Doxygen on the public part of the final API at least. Internal details aren't needed.

[kjbracey]

MyEnum cat = MyEnum::HasNose | MyEnum::HasEars | MyEnum::HasFur;

This is still quite annoying. C++20 will eventually come to the rescue with:

using enum MyEnum;
MyEnum cat = HasNose | HasEar | HasFur;

[kjbracey]

Oops, some corrections to my understanding of that other implementation. Its extra safety relies on everyone being clear on the "enum" versus "bitmask" distinction, so the usage model would actually need

Bitmask<MyEnum> cat = MyEnum::HasNose | MyEnum::HasEar | MyEnum::HasFur;

Using just MyEnum cat = ... would fail to work. That is strict, and does mean the end result doesn't model the C++ BitmaskType concept, which specifies that the values are of the same type as the object, so that doesn't permit the extra checks.

Still, it makes sense to me. It addresses the complaints of anyone who finds it weird to store values that weren't explicitly enumerated in an enum - you're not doing that any more, the raw MyEnum type is only the list of bits.

[pea-pod]

Sorry for the long delay. This is important to me, just not the most important thing right now. In any case, I have a few follow up questions from what you have written, @kjbracey-arm.

First, I am no lawyer. Mr. Aubut-Lussier's code is BSD licensed, but his license calls out attribution requirements. If I cut and paste, how would I attribute? Also, if I modify it somewhat (which I believe would be useful to make it prefixed with "MBED_", and possibly a slight change here or there) how would the license at the top of the .h file read?

My second question concerns the implementation. In my other code, the enum is stored in at least a 16-bit backing field (and I believe would be turned into a 32-bit word). The LSB is the read/write permissions, and the next byte is the opening method. The bits are orthogonal to each other. So were I to bitwise-or the Read flag with the ExclusiveCreate flag, would I be able to pass this to the init class method as I have it written in #12759?

Do we really want to restrict the comparison of masks and enums with the equality operator? Is this even that meaningful? In other words, I have not seen this sort of restriction in the other languages which include an enum of some sort (not considering C for this). Is this restriction overly strong?

Main annoyance of that enable form there is that the "enable" has to be placed at top level, not in any namespace your enum is defined in. But if you get that wrong, it is a compile error, not silent.

This is not a "deal breaker", as the kids say, but it seems the strongest argument against it. Are you saying that the enable macro would always have to be called outside of a namespace or is that just to wrap an extant enum class?

This seems a tad bid cumbersome:

namespace Gamgee {
enum class PoTayToes {
    BoilEm         = (1 << 0),
    MashEm         = (1 << 1),
    StickEmInAStew = (1 << 2)
};
}
MBED_ENABLE_ENUM_CLASS_BITMASK(PoTayToes);

but maybe I'm being too picky.

Finally, he has several structs defined in his code. I am a little concerned about adding more structs because C++ structs are classes and classes can grow. Also, would not the 4 different structs grow the required ROM or RAM in some respect? I suppose it could all be optimized to generate the same amount of bytecode as if it were the simple overrides I have MACRO'd up, but it is not clear to me that it would. That would be one advantage of my method: it would be a 4 byte chunk of data (although presumably it could possibly be an 8 byte in the worst case for ~0x0ULL). The compiler would not generate the 4 different structs. I understand why he is doing that (you have to have it thusly so your equality operator is now a class member and now you get a boolean result), although I am not familiar enough with his template party to get into the "how" exactly.

So those are my questions. I am happy to defer, although I would like to a. get something like this included for my own usage and have something consistent across the platform. That was my main purpose for attempting this in the first place in that I knew this could see use more in the whole mbed suite.

I accidentally edited this, rather than replying to it. Think I've put it back. Doesn't seem to be an easy "undo" in GitHub. --kjbracey-arm

[kjbracey]

First, I am no lawyer. Mr. Aubut-Lussier's code is BSD licensed, but his license calls out attribution requirements. If I cut and paste, how would I attribute? Also, if I modify it somewhat (which I believe would be useful to make it prefixed with "MBED_", and possibly a slight change here or there) how would the license at the top of the .h file read?

Maybe @0xc0170 comment on the licensing?

My second question concerns the implementation. In my other code, the enum is stored in at least a 16-bit backing field (and I believe would be turned into a 32-bit word). The LSB is the read/write permissions, and the next byte is the opening method. The bits are orthogonal to each other. So were I to bitwise-or the Read flag with the ExclusiveCreate flag, would I be able to pass this to the init class method as I have it written in #12759?

enum class always has type int if you don't specify, so it's 32-bit in Mbed OS. It doesn't autosize based on values like enum. If you need smaller or bigger you'd need to say so explicitly like enum class X : long long.

For both types of enum, it's always fine to OR together any values you validly put in its definition and store the result back in after a static_cast. From a language point of view at least. (Purists may hold that they don't expect an enum to hold values other than those listed, but it's legal).

Do we really want to restrict the comparison of masks and enums with the equality operator? Is this even that meaningful? In other words, I have not seen this sort of restriction in the other languages which include an enum of some sort (not considering C for this). Is this restriction overly strong?

I agree with the author that if (flags == ExclusiveCreate) is a semi-common error worth trapping. Most people writing something like that don't mean it.

But on the other hand, it means we're producing something stricter than the C++ BitmaskType concept.

I'm not 100% sold on it.

Main annoyance of that enable form there is that the "enable" has to be placed at top level, not in any namespace your enum is defined in. But if you get that wrong, it is a compile error, not silent.

This is not a "deal breaker", as the kids say, but it seems the strongest argument against it. Are you saying that the enable macro would always have to be called outside of a namespace or is that just to wrap an extant enum class?

This seems a tad bid cumbersome:

namespace Gamgee {
enum class PoTayToes {
    BoilEm         = (1 << 0),
    MashEm         = (1 << 1),
    StickEmInAStew = (1 << 2)
};
}
MBED_ENABLE_ENUM_CLASS_BITMASK(PoTayToes);

but maybe I'm being too picky.

You could maybe have an all-in one-shorthand that does that as well as the separate enable, but the existing shorthand form didn't handle enum class blah : uint8_t, so you'd need something to handle that too. And I'm not sure if there are any other variations you'd have to field.

Last time I tackled something like this was ns_list.h - there you'll see a pile of various short-hands for declaration/defining/typedeffing and all-in-one. The covering-95% shorthand shouldn't be the only thing ,there should be some maximally flexible form that can handle anything.

Only thing is you couldn't produce that enable with simple macros, due to the namespace.

That enable form in global namespace appears to be the one that the C++ committee have settled on for their own extensions, so I'm inclined to use it.

There was a suggestion in the comments of the Anthony Williams article that you could also allow enabling via a constexpr bool enable_enum_class_bitmask(PoTayToes) { return true; } in the Gamgee namespace - then argument-dependent lookup (ADL) would find that overload. Both enables could co-exist - turning on either works. And that function one could be done via an all-in-one macro.

Finally, he has several structs defined in his code. I am a little concerned about adding more structs because C++ structs are classes and classes can grow. Also, would not the 4 different structs grow the required ROM or RAM in some respect? I suppose it could all be optimized to generate the same amount of bytecode as if it were the simple overrides I have MACRO'd up, but it is not clear to me that it would. That would be one advantage of my method: it would be a 4 byte chunk of data (although presumably it could possibly be an 8 byte in the worst case for ~0x0ULL). The compiler would not generate the 4 different structs. I understand why he is doing that (you have to have it thusly so your equality operator is now a class member and now you get a boolean result), although I am not familiar enough with his template party to get into the "how" exactly.

It should all be optimised. Each value is only one type at a time, so only occupies one struct space. And all the operations will be inlined except in -O0 builds. There won't be a stack of different function definitions for each type.

However there is one issue in the ARM ABI, in that structure return is less efficient than scalar return. Just putting an int into a struct does make it less efficient. That only effects actual ABI calls that aren't inline, but that does mean that a function returning Bitmask<PoTayToes> would be less
efficient than one returning PoTayToes. MyStruct X(int); compiles as if it was void (MyStruct *, out), writing the struct to an output pointer, even if the struct contains just an int that would fit in R0.

I noticed this while working on Chrono - returning microseconds is less efficient than returning uint64_t. It's not in x86, but it is in ARM. :(

I think that may be the one final argument that dissuades me from going down that deeper route.

But that does rule out the automatic bool conversion too. You can't define explicit operator bool(PoTayToes) standalone. (For reasons of sanity you're not allowed to make up conversion operators outside either the source or destination class, and you can't modify bool, and you can't add member functions to an enum).

Given all that, you're basically back to something like Anthony Williams simpler template party, or your macros, and they're basically equivalent, both being BitmaskTypes. I'm personally inclined to the template form, but it's no big deal, and I bet others would prefer macros.

So I think we can proceed with your basic form here, but you will need some variant to handle underlying type specification, and make sure you can enable separately in some way. I'll review in more detail later.

[0xc0170]

Regarding the license, the file in question is licensed under "BSD 2-Clause "Simplified" License". just keep the license. You are allowed to make modifications. Once this is ready we can review the license in the file in question in this PR and update it accordingly. Will help with that.

[pea-pod]

First, I apologize for the huge delay. My intentions did not fit my actions well enough.

I could work out the macros myself, but I have much less experience with the templates. Could you either point to an example or give me one of such things? The only thing I could come up with seemed to end up giving bitflag powers to all enum classes everywhere. Obviously, that's not ideal.

Could you possibly provide me with some breadcrumbs toward how a template implementation might work?

[kjbracey]

In case my last comment wasn't clear, I'm fine with a macro implementation:

I think we can proceed with your basic form here, but you will need some variant to handle underlying type specification, and make sure you can enable separately in some way.

At the minute your macros don't let you choose the underlying type when defining the enum, and don't let you add the methods for an already-defined enum.

[pea-pod]

Ok. No problem. I have an easier time understanding macros themselves, but I know that the compiler is supposed to give you better error messages when a template barfs.

I will noodle it only a little more and then push an update shortly.

Thanks again for your help.

[pea-pod]

Well, alright @kjbracey-arm, I believe I have satisfied your requirements. It did turn into a macro a-splosion a little bit there, but I do not believe I repeated myself once. I wish I had remembered to astyle it first, but oh well.

Let me know if you have any questions.

[kjbracey]

I quite like this. Just needs a few tweaks. There are fair few style issues - satisfy astyle first, then I'll see if there's anything left.

Pull request has been modified.

[pea-pod]

So, @kjbracey-arm, is there anything else remaining before acceptance? I believe I have made all of your suggested changes. However, if I have missed something, please let me know.

[kjbracey]

Seems astyle is being lenient with you, presumably because of the macros. Quite a few excess spaces inside parentheses and <>. Here and in some other places.

[pea-pod]

I'm just making sure the macro expansion actually expands macros correctly. I will remove every space to make it virtually the same as a normal function.

[kjbracey]

I can't think of any way spaces and macros interact. Tokenisation happens before macro substitution of tokens. There's no way tokens can get "accidentally" pasted together.

[kjbracey]

Doesn't match the constructor form of the binary op now. More excess spaces. (Don't need spaces to separate >> any more.

[pea-pod]

Doesn't match the constructor form of the binary op now.

Oops!

More excess spaces. (Don't need spaces to separate >> any more.

I was being paranoid. I figured working was better than pretty. I was not sure I would not cause it to implode if I removed spaces. I was sure if I did have spaces and it worked with gcc, it should work for each supported compiler.

[kjbracey]

>> is special - there needs to be a magic "tokensiation" rule that gets it treated as > > in templates. That was added as a bit of a hack in C++11, but it's standard.

[kjbracey]

Needs an extra ; now.

[pea-pod]

Thanks!

[kjbracey]

REFERENCE. Slightly indirect name though? The reference isn't the point. From standard terminology they could bee"binary operator" and "compound assignment operator".

[pea-pod]

Ok, with this one, I was thinking of it not that in such a way as it only applied to enum classes that we wanted to make into bitflags. If I wanted to define the addition operator or the comma operator, I could do that with this same macro, as the signatures (with the exception of the exact symbol) are identical, and the name would still fit.

So in this case, I had ENUM_FLAG as a prefix, and OPERATOR and OPERATOR_LHS_REFERENCE as suffixes. But I do not mind narrowing down their usage, as this really is an internal macro anyway.

[kjbracey]

Okay, I see how you were thinking. Not that bothered :)

[kjbracey]

No space between & and variable name. Can probably have one before operator if you like.

[pea-pod]

I looked at the build log and it showed a failure for ASYNCHRONOUS_DNS_TIMEOUTS. I looked at it, but I could not determine what it could be. I would assume that a MBED_STATIC_ASSERT related error would fail during the compilation stage.

Digging through the error log shows this as 0x80010137 error code. Here is the link to the log and the crash dump.

I have a Nucleo-F767ZI and a Nucleo-F429ZI. If this is a true issue (and not with the CI environment) is there a way to short-circuit the mass of tests and just do the specific DNS timeout one before moving to doing all tests on my own computers?

[pea-pod]

@0xc0170, could you let me know if my code is truly in error or of so, how to execute this one test (or if not possible to narrow it down to the ASYNCHRONOUS_DNS_TIMEOUTS issue)?

Also, for everyone, regarding my earlier comment about MBED_STATIC_ASSERT, I was confusing my two different current PRs. I am still unclear about how this code would affect that chunk. Is it possible there is an issue with the CI system? It does happen to try three times before it gives up. Still, I am not sure where to start looking. Any suggestions?

[kjbracey]

Failure can't be related to this PR - I'm guessing it's some sort of race condition in socket closure on InternetSocket::_event_flags that the DNS tripped over. Not seen it before myself - must be rare?

Will retrigger the tests.

[mbed-ci]

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 2
Build artifacts

[adbridge]

Re-running CI as this passed 7 days ago

[mbed-ci]

Test run: FAILED

Summary: 1 of 6 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

• jenkins-ci/mbed-os-ci_cloud-client-pytest

[pea-pod]

@adbridge @0xc0170 I have searched a lot for the cause of this error and I cannot find it. I have seen other recent PRs fail this specific test, but pass on retest without any changes to the code.

If you agree with this understanding, could you kick off the test again?

[adbridge]

CI restarted

[mbed-ci]

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 4
Build artifacts