3

I have been using enumerations for function input parameters, but I have noticed that this may be very dangerous.

For example:

enum class MYENUM {
    X1 = 0, X2
};

std::map<MYENUM, int> mymap;
//init mymap here with known enum values of X1 and X2


int MyFunc(const MYENUM& input) {
    return mymap.at(input);
}

int main() {
    MyFunc(static_cast<MYENUM>(10000));
}

So 10000 is not a valid enum value. How would you personally find a solution to this? Would you encase the map access in a try block and catch the exception? What if I have many functions which access container information with user input enum values - would you then put try catch in all of them too?

Jan Schultke
  • 17,446
  • 6
  • 47
  • 96
expl0it3r
  • 325
  • 1
  • 7
  • Does this answer your question? https://stackoverflow.com/questions/4969233/how-to-check-if-enum-value-is-valid – cigien Aug 19 '20 at 16:32

3 Answers3

2

How would you personally find a solution to this?

Easy. One of the reasons for class enums introduction was type-safety - they would not accept any value automatically. So, the code you have here

MyFunc(static_cast<MYENUM>(10000));

Just circumvents the safety which was offered by the language. The solution is simple - do not circumvent the safety by casting, and the compiler will not allow you to use incorrect values.

And if someone else uses the cast - this is not of your problem. There are endless ways someone can bypass built-in type safety and provide completely bogus values to the functions. There is nothing library writer can do about it.

SergeyA
  • 61,605
  • 5
  • 78
  • 137
  • Technically there's nothing wrong with using enums like this as long as you know what you're doing. `std::byte` is an `enum class` which is being used in exactly this way. It's also common practice to combine multiple enums using `|` when passing them to a function as flags. – Jan Schultke Aug 19 '20 at 16:21
  • @J.Schultke those flag-enums a better not be class enums, as they are nothing more than a named constant. – SergeyA Aug 19 '20 at 16:23
  • 2
    They are. The only difference between `enum class` and `enum` is that `enum class` doesn't pollute global namespace. Otherwise they are identical in every regard. Usually an `operator|` is implemented for these flag enums which takes no more than three lines. – Jan Schultke Aug 19 '20 at 16:25
  • 1
    @J.Schultke, that's just an incorrect statement. regular enums do not require casts to convert from enum value to integral and back. – SergeyA Aug 19 '20 at 16:52
  • Perhaps identical in every regard was the wrong way to put it. *They are functionally identical*, meaning they will compile down to the same machine code and you can use them interchangeably if you implement an operator or two and avoid implicit conversions. – Jan Schultke Aug 19 '20 at 16:55
  • 1
    @J.Schultke I have never spoken about machine code, and I am not interested in this argument. I maintain that class enums are best suited for cases when you want to deal with distinctive and singular values, while non-class enums are best suited for named constants (as they have some slight advantages over constexpr variables). – SergeyA Aug 19 '20 at 16:58
  • I can't entirely agree with this answer. Absent crucial performance reasons, well-written software _should_ perform range checks against "invalid" or unexpected input, if the alternative is a dangerous abort (e.g. the uncaught exception thrown by `std::map::at`) or, worse, UB things. Only the OP can decide whether or not that's appropriate in their use case, but your answer quite firmly asserts that it never is. – Asteroids With Wings Aug 19 '20 at 18:03
  • (I concede that this is less obvious for a scoped enum than for an unscoped one) – Asteroids With Wings Aug 19 '20 at 18:05
  • @AsteroidsWithWings a class enum mandates a contract, and there is nothing else any sensible library writer is expected to do. If someone tries to send a value which is not part of the class enum through cast, it is not something a library author can control or should be expected to somehow 'protect' from. This is not different from passing an invalid pointer to function expecting a pointer input - a function can not generally verify pointer validity (except in a couple of narrow cases, most notably, null pointer). – SergeyA Aug 19 '20 at 18:11
  • _"there is nothing else any sensible library writer is expected to do"_ There is, I already said: exchange terrible results for better results. Throwing a documented exception or returning some sentinel value is safer than just shrugging and aborting. Be a good software citizen. Help your users find their bugs. Don't just spit at them! You don't need to _spite_ someone for violating a contract, if it costs you nothing to fail gracefully instead. Of course, you're allowed to, and that's where the _choice_ comes in (which you did not offer the OP). – Asteroids With Wings Aug 19 '20 at 18:11
  • And it's _completely_ different from somehow verifying pointer validity (mainly because that is not possible) – Asteroids With Wings Aug 19 '20 at 18:13
  • @AsteroidsWithWings, no, it is the same thing. It is all about contract and a violation of thereof. Here class enum defines contract in the best possible way. It is responsibility of the caller to not violate it. Embrace undefined behavior, this is the core principle of C++ (unlike other languages), and it is expressed in it (just count the number of occurrences of 'undefined' or 'ill formed, no diagnostic is required' in C++ standard. While you are certainly entitled to your opinion, this is not the opinion shared byh C++ committee. – SergeyA Aug 19 '20 at 18:16
  • Looks like we'll have to agree to disagree; certainly there's no point in me repeating what I've said over and over again. :) But, as a consequence, it'll have to be a -1, sorry. You're not writing the C++ standard and it doesn't matter whose "responsibility" it is. Your code must be terrifyingly fragile, and for literally no reason other than spite. Sad! – Asteroids With Wings Aug 19 '20 at 18:16
0

So 10000 is not a valid enum value

That may be true, but not because it's not equal to X1 or X2. An enum doesn't have to hold one of the named enumerators. It can hold any value in its integer range.

Now, the actual range of MYENUM is up to the compiler, as long as the integral underlying type it picks can represent both X1 and X2. So, it could be a int8_t, for example, which cannot hold 10000.

(In practice, it's probably an int here.)

How would you personally find a solution to this? Would you encase the map access in a try block and catch the exception?

Yes, if you like.

There are many approaches to error handling for functions like this, whether they involve enums or not. You can throw an exception (the .at() call will already do this for you, but you could catch it and throw an exception of your own type if you liked).

You could populate an error code, return a placeholder value, return a std::optional… or just ignore it, and document that the user of your function must pass a named member of the enum, or fall foul of a sort of "undefined behaviour" (possibly choosing to have the function act as if X1 were passed in that case, so that it has something to do at least).

All the usual options.

What if I have many functions which access container information with user input enum values - would you then put try catch in all of them too?

That is indeed a possible consideration to bear in mind when choosing your input argument error handling methodology.

Asteroids With Wings
  • 17,071
  • 2
  • 21
  • 35
-1

There is nothing inherently dangerous about enum and function signatures with enum parameters are not bad practice. In your particular example, 10000 is still a valid enum, just not one that has a name.

This only becomes a problem when looking up in a std::map<MyEnum, Something> or using a switch (myEnum). To mitigate these problems, you could perform a sanity-check for your enum values:

// specify a type for MyEnum explicitly so that we know we can fit values
// such as 100000
enum class MyEnum : unsigned {
    X1 = 0, X2
};

// On a side note, enums are so small in size that it makes no sense to
// pass them by reference, simply pass them by value everywhere.
constexpr bool sanityCheck(MyEnum e) {
    return e == MyEnum::X1 || e == MyEnum::X2;
}

int MyFunc(const MyEnum& input) {
    assert(sanityCheck(input));
    return mymap[input];
}

You could even create an assert function that only compiles on release builds to have zero runtime cost. This will be much more effective than putting a try-catch everywhere.

But the best solution is to simply be careful when you create MyEnum using a static_cast. Don't pass the result into functions accepting MyEnum if you're not 100% sure that the value has a name.

Jan Schultke
  • 17,446
  • 6
  • 47
  • 96
  • `10000` might not actually be a valid `MyEnum`. I believe that cast is undefined behavior, as `MyEnum` might only be able to hold values between 0 and 1 on some compilers. https://eel.is/c++draft/dcl.enum#8 – Mooing Duck Aug 19 '20 at 17:50
  • 2
    @MooingDuck it's not undefined behavior, it's implementation-defined behavior. Other than that you're right. Most compilers I've seen use `int` for `x86` though, because it's the most convenient on `x86`. On other platforms the underlying type might be different by default. – Jan Schultke Aug 19 '20 at 17:53