refactor(enginebuffer): Make EngineBuffer::KeylockEngine an enum class

[Swiftb0y]

No description provided.

[ronso0]

well, thanks!
I'm still a c++ noob so I need someone else to review this. @daschuer could you take a look?

[daschuer]

This duplication is a source of later error.

I am not against using an enum class here. But just because this is a good example how software paradigms and maintainability/readability are not always the same, let me share my view:

enum classes have been introduced for two reasons

• A protected namespace
• type-safty

Part of the type safety is that the value representation can be random. There is no iterator, no compare operator and no ++ operator or implicit numeric value cast because of that reason, this protects the developer from doing wrong.

However, in our case we need exactly this. The enum is in fact just a util to not deal with the magic numbers from the mixxx.cfg file in our source code.

We need control over the value, we need the integer presentation and an iterator, the sings that have been removed from the enum class by design. So from the type-safty aspect an plain old Enum is a better fit ... but without the protected namespace we reintroduce a "strong" issue.

So I propose to reintroduce just the value KeylockEngine::Count and the integer loop in this case.

The alternative would be to add the missing operators to this enum class https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Renum-oper or introduce a generic solution like shown here: https://stackoverflow.com/a/8498694

[Swiftb0y]

Your review is reasonable and I have a couple notes:

So I propose to reintroduce just the value KeylockEngine::Count and the integer loop in this case.

I strongly object to this. As you said yourself, we can't be sure about the underlying value. Adding a last entry and assuming it indicates how many constants the enum contains is IMO a bad code smell. The current approach is very careful to not cause undefined behavior. As such, casting from some int to KeylockEngine was carefully avoided. We can't assume that the enum values start at 0 and are consecutive. For example if we ever removed Soundtouch, we can't remove the Soundtouch entry from the enum. We'd also have to modify the loop that loop that does the static cast hack, etc. All of which could cause bugs and UB and from what I can tell, the compiler can't warn us about.

So we need an iterator, if I were to define an iterator on the Keylock enum, the result would essentially be another giant switch-case mess, because due to the reasons I mentioned above, static_cast<KeylockEngine>(++static_cast<int>(engine)) won't work.

The problem with the value representation being random is also not a problem when provide explicit values (as suggested in my own comment above).

My initial plan was to make kKeylockEngines be kAvailableKeylockEngines and then refactor the other querying methods in terms of that, but that either resulted in unnecessary interdependencies or complex code that would be objected to.

This duplication is a source of later error.

Most of the times yes, but here, the duplication is right next to each other and has a clear purpose.

[daschuer]

We actually need an int below in this loop, so there is not much a point of using a range based loop.

[Swiftb0y]

As pointed out above, I didn't want to do that because I wanted to avoid the cast from int to Keylock engine when I couldn't be sure that the values actually came from Keylockengine in the first place. So this solution is somewhat more typesafe.

[Swiftb0y]

Actually, the problem is that we're converting to an int so we can use it as a QVariant, but throwing the enum value into QVariant right of the bat would make much more sense. Let me fix that.

[Swiftb0y]

fixed

[daschuer]

LGTM, Thank you.

I think our different view is sourced by the question what is the leading value type. Is it the constitutive integer from the mixxx.cfg or the Enum Class managed by the compiler. Before you last change we where on the middle ground. Now you have pushed it completely to the later.
This is OK to me.

However let's clarify one thing: In C++ the integer representation of of the enum is well defined:
https://en.cppreference.com/w/cpp/language/enum

Each enumerator is associated with a value of the underlying type. When initializers are provided in the enumerator-list, the values of enumerators are defined by those initializers. If the first enumerator does not have an initializer, the associated value is zero. For any other enumerator whose definition does not have an initializer, the associated value is the value of the previous enumerator plus one.

This means, adding a last value and use it in an iterator is also a valid and reliable solution.

@ronso0 You may hit merge now. Thank you.

[ronso0]

Great, thanks!

@Swiftb0y wanna squash the fixup or should I squash all commits?

[Swiftb0y]

This means, adding a last value and use it in an iterator is also a valid and reliable solution.

Well sort of. Yes, the semantics are fortunately well defined, but that doesn't mean that its still a good solution. As I pointed out, it could be in the future that the enum values become non-consecutive for some reason. So that't why I wanted to avoid the consecutive iteration. I also don't like that approach semantically. IMO an enum is supposed to group related constants together. So the length of the enum is not related to the rest of the constants and thus should not be an enumerator. IMO this is somewhat of a short-coming in c++. There should be a std::size specialization for enum classes or something.

Before you last change we where on the middle ground. Now you have pushed it completely to the later.
This is OK to me.

Can you elaborate? I didn't want to bully you into accepting my solution. Did we come to consensus we are both ok with?

wanna squash the fixup or should I squash all commits?

I kinda messed up the fixup. I'll just squash everything into a single commit.

[daschuer]

Can you elaborate? I didn't want to bully you into accepting my solution.

You solution is now good, since you did the extra mile. Thank you.

[Swiftb0y]

Thank you for the review.

[ronso0]

🎉 Thanks to both of you!