Skip to content

ES.79 is a terrible advice and should be dropped #2163

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hasselmm opened this issue Dec 18, 2023 · 28 comments
Open

ES.79 is a terrible advice and should be dropped #2163

hasselmm opened this issue Dec 18, 2023 · 28 comments

Comments

@hasselmm
Copy link

hasselmm commented Dec 18, 2023

ES.79 is a terrible advice that will cause unreliably code and serious bugs once project evolve and new enum values get added. Adding a default case to switch statements blinds compilers regarding new enum values (unless the proper compiler with the proper magic switches is used). By adding a default case to a switch statement one gives up the compiler's ability to support us to write correct code without good reason. This rule should be dropped, or even reverted into "do not ever use default: and lobby the ISO consortium to remove it form the language".

@jwakely
Copy link
Contributor

jwakely commented Dec 18, 2023

ES.79 is a terrible advice that will cause unreliably code and serious bugs once project evolve and new enum values get added. Adding a default case to switch statements blinds compilers regarding new enum values (unless the proper compiler with the proper magic switches is used).

Why? The enforcement section says (emphasis mine):

Flag switch-statements over an enumeration that don’t handle all enumerators and do not have a default.

So if your local coding standards say that all switches must cover all enumerators, and your code already does that, then there's no need to add a default case. And if you add another enumerator, this guideline says that a tool enforcing the guideline should complain. Adding a new case for the new enumerator would satisfy the guideline. I assume that's exactly what you want? If not, try explaining your position properly.

"do not ever use default: and lobby the ISO consortium to remove it form the language"

lol, good one. Do you have any proper rationale for breaking millions of lines of working code?

Rationale for not doing that would be that it makes it impossible to use a switch with an enumerated type that has no enumerators, e.g. std::byte

switch(b) {
case 0xfe:
case 0xff:
  return std::make_error_code(illegal_byte_sequence);
case 0x0:
  return finish();
default:
  process(b);
}

Without default you would need to write 256 cases here. You could rewrite it as if-else but you shouldn't have to.

@hasselmm
Copy link
Author

Your example actually demonstrates perfectly why default is not needed at all: As you return from every known case you can just handle the unexpected or default case outside of the switch statement. Actually this is generally good practise to use switch statements for delegation only and to always return (or fallthrough) from known cases: This avoids to create god-functions with way too high complexitity.

@jwakely
Copy link
Contributor

jwakely commented Dec 18, 2023

Imagine there's another non-default case that doesn't return. In any case, default isn't going to be removed from the language, so that part of your issue is just silly.

Regarding the guidelines and actual constructive feedback, what part of ES.79 says you have to use default? What is the kind of code you want to write that ES.79 complains about? Why should we not just close this as lacking justification for any change?

@BenjamenMeyer
Copy link

BenjamenMeyer commented Dec 23, 2023

ES.79 is a terrible advice that will cause unreliably code and serious bugs once project evolve and new enum values get added.

lol... ensuring all your cases are properly handled is actually good coding practices. default is great for helping your code catch things - like when you added another value and the switch isn't handling it correctly. Good code debugs the software for you and points to you the issues instead of hiding them.

As you return from every known case you can just handle the unexpected or default case outside of the switch statement.

This is rather absurd. using a switch provides a high speed mechanism for branching. Are there limits? Yes - there are limits to how many entries can be in a switch table (which is a good thing too since too large of a table also makes it unreadable and more prone to errors), and in those cases it's easy to move from a std::map<> or similar to keep the near constant time. Adding more branching to handle boundary cases before the switch means slower performance and less optimization.

I'll re-iterate @jwakely and quote from ES.79 itself:

Enforcement Flag switch-statements over an enumeration that don’t handle all enumerators and do not have a default.

Emphasis mine.

@hasselmm
Copy link
Author

@BenjamenMeyer Nobody argues against handling errors. My problem really is that default: causes more errors than it prevents. It seemed like a good idea when introduced, but time has prooven this tool not being suiteable for its job. Instead of helping us to write reliable and secure code, it does quite the opposite. Simply not fit for the job. Time to retire that feature.

What really confuses me is your talk about performance. First you introduce the switch statement as performance tool, just to entirely destroy your own hypothesis when mentioning big tables and the better suited alternatives. So it seems you know, while not agreeing with your own conclusion, that switch is NOT a performance tool.

So what was the purpose of your comment, other than insulting me?

@hasselmm
Copy link
Author

Imagine there's another non-default case that doesn't return.

@jwakely I'd call such inconsistency pretty terrible code. It wouldn't pass my code review if I see it. I'd only accept such if there are unexpected, invalid states if it makes sense to handle them consistently to out of domain values. Tools can catch such bad style.

In any case, default isn't going to be removed from the language, so that part of your issue is just silly.

Is "this is silly" a valid and rational argument these days?

Regarding the guidelines and actual constructive feedback, what part of ES.79 says you have to use default? What is the kind of code you want to write that ES.79 complains about? Why should we not just close this as lacking justification for any change?

You start with this:

enum class Result { Success, FileError, FormatError };
std::tuple<Result, Model> processData();

void something()
{
    auto [state, model] = processData();

    switch (state) {
    case Result::Success:
        useModel(std::move(model));
        break;

    default:
        showError();
        break;
    }
}

Month, years after writing the code above someone introduces the new state Retry for better UX. The ES.79 compliant snippet above will never retry. Good luck with finding and fixing all of these cases in a mature code base.

Other example:

// Identifiers of some automotive hardware's fictional UDS/TP interface
enum class DataIdentifier { 
    Gear,
    Speed,
    Handbrake,
    Brake,
    // ...
    ContainerHookLocked,
    UnderRunProtectionInstalled,
    UnderRunProtectionActive,
    UnderRunProtectionPosition,
    UnderRunProtectionFoldLimit,
    UnderRunProtectionUnfoldLimit,
    // ... some 100 other parameters
};

// Terrible, fictional example.
// Fictional story: Updating data identifiers is expensive for automotive reasons.
// Therefore we only refetch these identifiers that are relevant for the next operation.
constexpr bool affectsMoveContainerArm(DataIdentifier id)
{
    switch (id) {
    case DataIdentifier::Speed:
    case DataIdentifier::Gear:
    case DataIdentifier::Handbrake:
    case DataIdentifier::ContainerHookLocked:
    case DataIdentifier::UnderRunProtectionActive:
        return true;

    default: // I've delete the full list: was too much work to add new identifiers and ES.79 encourages this
        return false;
}

Now after years successful of operation product management introduces a new truck body that replaces the single container hook by two hooks for... "reasons". They report via the newly introduced identifiers UDS ContainerHookLeftLocked and ContainerHookRightLocked. Poor and badly crafted affectsMoveContainerArm() will never report that these two identifiers must be refetched before deciding if the container arm can be moved. Insert nasty noise of bending metal here.

Right now we have banned default from use in our code base exactly for such expensive, and probably even dangerous mistakes. I really don't like that ES.79 lobbies for ending this ban

@jwakely
Copy link
Contributor

jwakely commented Dec 24, 2023

You've shown why you don't want to use default in your code. Fine. So don't use it. That's allowed by ES.79. It doesn't try to stop you banning default in your code.

I'm still not sure if you actually understand the guideline or you're just angry with default. You failed to answer my question: what part of the guideline says you have to use default? Please don't just show why you dislike it again, that's not the question and you don't need to explain that. Please explain why the guideline conflicts with your coding style.

@kcat
Copy link

kcat commented Feb 19, 2024

You failed to answer my question: what part of the guideline says you have to use default?

In the Enforcement section: Flag switch-statements over an enumeration that don’t handle all enumerators and do not have a default. If you don't handle all enumerators and don't have a default: case, the guidelines are saying to add a default: case instead of the missing enumerators. Which is terrible for the reasons stated. The guidelines give the reason "Code clarity. Improved opportunities for error detection.", but in reality it's the opposite: using a default: case decreases code clarity and reduces the opportunities for error detection.

IMO, it is better to avoid default: cases to allow the compiler to warn you about unhandled enumerators, to ensure enumerators are handled correctly rather than being silently funneled into a default catch-all that likely isn't going to do what you want for new enumerators. But having a default silences the compiler and code checkers, hiding bugs and making them harder to track down as you have to rely on catching undesired runtime behavior, and making it less clear what the code's intention is. Taking the guideline's own example:

enum E { a, b, c, d };

void f1(E x)
{
    switch (x) {
    case a:
        do_something();
        break;
    case b:
        do_something_else();
        break;
    default:
        take_the_default_action();
        break;
    }
}

"Here it is clear that there is a default action and that cases a and b are special."

But it isn't clear that a and b are special next to c and d. Does the developer intend for c and d to be handled by the default case, or were they added later and the switch wasn't updated to account for them? Are c and d not expected values in this code path, such that there would be an error elsewhere if they manage to hit, and the default: is just to shut up the compiler? And/or did the enum value come from an outside source without verification, and the default: case is there to catch invalid enum values? The developer didn't list c and d so there's nothing saying what the intention is for them here. The guidelines even touches on this issue in its default-less hypothetical:
"Did you forget case d or deliberately leave it out? Forgetting a case typically happens when a case is added to an enumeration and the person doing so fails to add it to every switch over the enumerators."
but adding a default: case doesn't clarify if d was forgotten or deliberately left out either, and sabotages attempts to detect where someone failed to add cases for d in switches.

It would be better instead to suggest adding the missing cases explicitly:

    switch (x) {
    case a:
        do_something();
        break;
    case b:
        do_something_else();
        break;
    case c:
    case d:
        take_the_default_action();
        break;
    }

This makes it clear that c and d take a default action separate from a and b. If a new enumeration e is added, your compiler/checker will alert you that this switch doesn't know what to do with e. This improves code clarity and opportunities for error detection. I do all I can to avoid default: cases in my serious (long term maintenance, non-toy) code, so that I know I'm accounting for all enumerators and will be warned at build time if another one is added that I don't handle, and use it only to deal with user/unknown input where I have to account for unexpected values.

Ideally we'd have strict enums, that is enums that cannot hold a value other than its specified enumerators without invoking UB, and maybe even case ranges (i.e. GCC's case c ... e:) that with properly named enumerator markers would allow making a case for a range of enumerators without having to list them all individually, while still being relatively clear.

But short of that, non-swallowing default: would be the next-most ideal scenario (i.e. a default: case that doesn't stop the compiler/checker from warning about unhandled enumerators). That way you can have the safety fallback in the case of an enum holding a value not matching an enumerator, but still be warned that you haven't specified cases for known enumerators.

@jwakely
Copy link
Contributor

jwakely commented Feb 19, 2024

You failed to answer my question: what part of the guideline says you have to use default?

In the Enforcement section: Flag switch-statements over an enumeration that don’t handle all enumerators and do not have a default. If you don't handle all enumerators and don't have a default: case, the guidelines are saying to add a default: case instead of the missing enumerators.

No, they're not saying that. You can add cases for the missing enumerators instead. I don't know why you think it's saying otherwise.

It says to add an empty default "if there is no default action and you mean to handle only specific cases" i.e. you do not want to handle all enumerators. That's talking about a different situation from the one you're describing. In your situation you want to handle all enumerators, so you would just add cases for all enumerators. Then you don't need a default.

Which is terrible for the reasons stated. The guidelines give the reason "Code clarity. Improved opportunities for error detection.", but in reality it's the opposite: using a default: case decreases code clarity and reduces the opportunities for error detection.

So add the missing cases for the other enumerators then.

IMO, it is better to avoid default: cases to allow the compiler to warn you about unhandled enumerators, to ensure enumerators are handled correctly rather than being silently funneled into a default catch-all that likely isn't going to do what you want for new enumerators.

So don't add a default case, and when you add new enumerators, add cases for those.

It would be better instead to suggest adding the missing cases explicitly:

So do that then. It will avoid the condition that the enforcement talks about, so would be following the guideline.

I don't understand why people keep interpreting this guideline to mean something it doesn't say.

@jwakely
Copy link
Contributor

jwakely commented Feb 19, 2024

N.B. if you have 30 enumerators and you only want to handle 5 of them in the switch, you're saying it would be better to do:

switch (e) {
case e1:
  blah1();
  break;
case e2:
  blah2();
  break;
case e3:
  blah3();
  break;
case e4:
  blah4();
  break;
case e5:
  blah5();
  break;
case e6:
case e7:
case e8:
case e9:
case e10:
case e11:
case e12:
case e13:
case e14:
case e15:
case e16:
case e17:
case e18:
case e19:
case e20:
  etc etc
default:
}

Rather than just:

switch (e) {
case e1:
  blah1();
  break;
case e2:
  blah2();
  break;
case e3:
  blah3();
  break;
case e4:
  blah4();
  break;
case e5:
  blah5();
  break;
default:
  // nothing to do for all other values.
}

That seems pretty dumb to me. In a case like this, adding the default case makes a lot of sense. If you don't have so many enumerators, or you like the first example above, feel free to add them explicitly. The guidelines allow either form, so you can choose.

@jwakely
Copy link
Contributor

jwakely commented Feb 19, 2024

The title of the guideline even reinforces it: "Use default to handle common cases (only)" i.e. don't use it if there isn't a common "everything else" case. So if you want to handle every enumerator explicitly and not have an "everything else" case, you should do that. Don't add a default for that situation, because there is no "common case" when you intend to handle each of them explicitly.

And if you want to be explicit about "all of these should be treated as the everything else case" then you can do that.

@kcat
Copy link

kcat commented Feb 20, 2024

No, they're not saying that. You can add cases for the missing enumerators instead. I don't know why you think it's saying otherwise.

Because that's what it's saying. In the section titled, "Use default to handle common cases (only)", enforcement is described as "Flag switch-statements over an enumeration that don’t handle all enumerators and do not have a default". I don't know how else to read this other than "if you don't specify all enumerators and don't have a default case, add or suggest to add a default case." I'm apparently not the only one that interprets it this way.

If the purpose of the "(only)" is to basically say "(but not always)" and warn people away from doing it, after realizing it wasn't so good of an idea to do by default, that further questions its inclusion as a core guideline.

It says to add an empty default "if there is no default action and you mean to handle only specific cases" i.e. you do not want to handle all enumerators.

But a default case doesn't express that intent. It doesn't say "I mean to handle only specific cases listed above, everything else goes here" to me, it says "I didn't want to list out other possible enumerators and I wanted to shut the compiler up, hopefully nothing bad happens when a new one is added". Maybe I'm too used to lazy coders, but if the goal of the guidelines is to help adoption of modern practices for safer and more maintainable code, ES.79 is working against that.

Even if it is for a common case, seeing a default is a red flag that it may not be handling the enumerators properly, it can be silently missing one that should be handled in a non-default way or can easily and silently break when a new enumerator is added in the future. Contrary to its own reason, ES.79 reduces code clarity and inhibits error detection. This makes ES.79 bad advice, IMO.

So add the missing cases for the other enumerators then.

Which should be the suggested rule/guideline IMO, contrary to ES.79. To prefer specifying missing enumerators instead of using default to catch all unlisted ones. A default case is best used in situations where an enum can have an unknown/"invalid"/non-enumerator value, while also having non-swallowing behavior so you still get warned about unlisted enumerators. But as most compilers don't have a non-swallowing default, it's unsafe and should only be used as a last resort.

N.B. if you have 30 enumerators and you only want to handle 5 of them in the switch, you're saying it would be better to do:
[...]

That's where case ranges and enumerator markers would be helpful.

enum Enum { e0, e1, BeginGroupFoo, e2=BeginGroupFoo, e3, /*...*/, e20, EndGroupFoo=e20 };
...
switch(e)
{
case e0:
    blah0();
    break;
case e1:
    blah1();
    break;
case BeginGroupFoo ... EndGroupFoo:
    etc etc
    break;
}

This more clearly indicates e0 and e1 are handled special, and everything in "GroupFoo" is handled with the same path. If another enumerator is added inside of GroupFoo, it's handled the same as all the others in the group as one would expect, whereas if another enumerator is added outside of it, you get a warning and avoid a bug early.

In contrast, a default case swallows everything that's not listed, leaving it unclear if any of the unlisted enumerators were meant to be handled. Without case ranges to collect enumerators into explicit groups, listing the individual enumerators would be the safer and clearer option, so it's clear what each is intended to do and ensure none of the enumerators are missed. It's not pretty, but being safe isn't always pretty.

@jwakely
Copy link
Contributor

jwakely commented Feb 20, 2024

"Flag switch-statements over an enumeration that don’t handle all enumerators and do not have a default".

That does not say "add a default". It says when static analysis tools should complain. If you add cases to handle all enumerators, the tools should not complain. So adding cases to handle all enumerators is a valid way to follow this guideline.

The rest of your answer is just repeating "default bad default bad" again. I get it, you've made your point. I don't agree default cases are always bad, but I get it. It's not hard to understand your position, you don't need to keep repeating it.

But the guideline does not say "always have a default case". It says "do not have unhandled cases". It's really that simple.

If you don't want to use default, don't. As long as you handle all cases in some form then analyzers should not say you're failing to follow ES.79.

Maybe the title or summary of ES.79 needs to be clarified for people misreading it but the issue says it's "terrible advice and should be dropped", based on ... something it doesn't actually say. "Do not have unhandled cases in a switch" is not terrible advice, and should not be dropped.

@kcat
Copy link

kcat commented Feb 20, 2024

Maybe the title or summary of ES.79 needs to be clarified for people misreading it but the issue says it's "terrible advice and should be dropped", based on ... something it doesn't actually say.

At least in my case, it's both the title and provided examples, along with reasoning.

The title sets the context, so the section is interpreted in that context. Here, the title is "Use default to handle common cases (only)", so the explanation of missing cases, the reasoning of code clarity and safety, and what to do is going to be read in the light of "Use default to handle common cases" (and the added "(only)" at the end is confusing, like it's an incomplete aside statement; "only ... what?").

It harps on using default to prevent problems, how the first example is "clear that there is a default action and that cases a and b are special" (which is debatable), and for the second example where you don't need to do anything for some cases, it says "have an empty default or else it is impossible to know if you meant to handle all cases".

The section just reads as "use default, or else problems", not "handle all enumerators, either explicitly or with default". "Handle all enumerators" isn't mentioned until the end in the Enforcement paragraph, after the reader is told to use default and the problems of leaving out a default. Whether it intends to or not, ES.79 comes across as "Use default in your switches", which I would indeed call terrible general advice (I won't go as far as to say it should never be used or removed from the language, but I don't think a swallowing default should be called clear and safe, either).

@AndrewBloom
Copy link

AndrewBloom commented Jan 30, 2025

Ideally we'd have strict enums, that is enums that cannot hold a value other than its specified enumerators without invoking UB, and maybe even case ranges (i.e. GCC's case c ... e:) that with properly named enumerator markers would allow making a case for a range of enumerators without having to list them all individually, while still being relatively clear.

  • what happens if c1 is added after c?
  • what happens if someone decides to reorder the sequence in the enum definition as e d c b a or c e a d b?

@kcat
Copy link

kcat commented Jan 31, 2025

what happens if c1 is added after c?

Then c1 is included in the range. Such ranges would be indicative of logical groups, so adding a new value within the group will include it in the range. Compared to default:, which will implicitly take any new value added to the enum, a case range would only implicitly take new values added within that range.

what happens if someone decides to reorder the sequence in the enum definition as e d c b a or c e a d b?

IMO, it should be an error to define a backwards range case, so if you have case c ... e: and someone modifies the enum to put e before c, the case will cause a compile error. Alternatively, the range could be order-independent, e.g. case c ... e: and case e ... c: would be equivalent (essentially case min(c,e) ... max(c,e):). The latter could potentially be confusing though, and potentially error prone if used in templates, so I think the former option is better.

If the values are reordered to move a d b outside of the c ... e range, then they're no longer in the group and would be excluded from the range. Without a default: case, the compiler will then warn that the a d b cases are no longer being handled, indicating to add appropriate cases for them if they're not part of the group, or modify the case range to work with the new order if they are still a logical group. If you do have a default: case, then just like if you added new values after e, it would implicitly handle them. The fact that this then becomes ambiguous if you meant for default: to handle the values now outside of the range is exactly my issue with using default: to handle unspecified enum value cases.

@AndrewBloom
Copy link

AndrewBloom commented Jan 31, 2025

Such ranges would be indicative of logical groups

In practice enum values can be part of different logical groups, and logical groups have non empty intersections.
As example, in an enum of computer peripherals, a mouse would be both in the input logical group and usb logical group. a switch in function1 may need to group for usb while another switch in function2 may need to group for input. I don't see it working in practice.

for point 2, reordering to c a d e b doesn't trigger any compiler errors tough. A subtle dependency on the order looks even worse than a default: imo.

@jwakely
Copy link
Contributor

jwakely commented Jan 31, 2025

So you wouldn't use case ranges with all enum types 🤷‍♂️

Not every feature needs to be applicable in every situation.

@jwakely
Copy link
Contributor

jwakely commented Jan 31, 2025

The same problem exists with if (a <= x && x <= b) and yet we manage.

@AndrewBloom
Copy link

AndrewBloom commented Jan 31, 2025

So you wouldn't use case ranges with all enum types 🤷‍♂️

Not every feature needs to be applicable in every situation.

Totally agree, my thought was that case ranges behave exactly as default: regarding preventing new values being mismanaged (if not worse) because they introduce a dependency on enum values order, which is something that enums use should not rely on.

The same problem exists with if (a <= x && x <= b) and yet we manage.

and you could say: default: The same problem exists with if (a <=x && x<=b) do_something() else process_default() and yet we manage.

Here the OP argues that the language should enforce correctness at compile time between two different versions of the code (the starting version and the one modified by a sloppy developer) against changing of values of type enum T. I think this is outside of scope of the language definition, and instead be the domain of tools. The language should only ensure correctness of the code as it is on a fixed moment. default: does not violate that. the code is well defined, and if it is not what the developer intended, then it's the developer fault.

@kcat
Copy link

kcat commented Feb 1, 2025

and you could say: default: The same problem exists with if (a <=x && x<=b) do something() else process_default() and yet we manage.

Incidentally, one of the reasons given to use a switch() instead of a series of if(x == a) .. else if(x == b) ... when possible is because the latter can be error prone. With x being an enum type, you don't get a warning about not handling some value x could be when using if...else if..., an issue that gets reintroduced when using default:. There are other reasons of course, such as avoiding multiple evaluations of x (a switch only evaluates it once), so it's not the only reason to prefer a switch when possible.

the code is well defined, and if it is not what the developer intended, then it's the developer fault.

The point of the C++ CoreGuidelines is to provide a set of good practices to minimize error, and to provide a set of rules that tooling can use to point out potential problems, all to prevent unintentional behavior aka bugs. The OP's final line about "or even reverted into "do not ever use default: and lobby the ISO consortium to remove it form the language"" is a bit much, sure, but the underlying issue of default: silently taking all remaining unspecified cases, and it not being clear if every unspecified case being handled by default: is intentional (contrary to the guideline's assertion that a default case makes the intention clear), does have a rational basis.

And the argument I made for case ranges is basically that A) it provides a simple mechanism to handle a large set of cases with the same handler without having to list each one individually, reducing the need for a default case, and B) is less harmful than a default case as it only takes values within the specified range, as opposed to "everything else not specified", so you can still get warnings about unspecified cases.

@AndrewBloom
Copy link

Incidentally, one of the reasons given to use a switch() instead of a series of if(x == a) .. else if(x == b) ... when possible is because the latter can be error prone.

Sorry, I actually was referring to any generic if-else use, not just the if-else chains. On a 'more philosophical' level, everytime you use else you are basically saying for all the other cases. This is open to someone introducing a meaning for one of those cases in a later stage and requiring management for that, and the consequent modification of the code in all the affected parts. Values are checked at run-time, when compile-time checking is delegated to the type system. Nobody prevents you to put a log on the default: case, or throw or abort.

In the example that was shown by the OP,

// Identifiers of some automotive hardware's fictional UDS/TP interface
enum class DataIdentifier {
Gear,
Speed,
Handbrake,
Brake,
// ...
ContainerHookLocked,
UnderRunProtectionInstalled,
UnderRunProtectionActive,
UnderRunProtectionPosition,
UnderRunProtectionFoldLimit,
UnderRunProtectionUnfoldLimit,
// ... some 100 other parameters
};

what catches my attention is that line, // ... some 100 other parameters. It looks like that all those values are used to characterise a type, without using the type system. Basically it's passing heterogenous data to functions, that should be instead filtered and assigned to different types. With different types, when you call the functions, you don't have hundreds of different values, but just a few possibilities left for discriminate a specific type, and you drastically reduce the number of functions called that use a specific enum for a specific type. This to me smells of bad architectural design, and it's not a problem of adding a ban of default as coding rule, it's the problem that the project has the wrong architect.
IMO, an enum should have few values used in few places, not hundreds of values used in hundreds of switches. In fact, when they have a reasonable number of values, a switch can work as intended as

a high speed mechanism for branching.

And the argument I made for case ranges ... is less harmful than a default case as it only takes values within the specified range

As I said, I respectfully disagree and see case ranges much more harmful because they introduce a dependency on the order of the enum values.

Said that, ES.79 doesn't prevent you not to use default if you wish not to. The Enum.2 - Enforcement too distinguishes 2 cases.

Finally, let me thank you for giving me the chance to learn and delve more into the subject and sharing your point of view with me and the community, and excuse me if I comment (off-topic) on the OP sentence:

I'd call such inconsistency pretty terrible code. It wouldn't pass my code review if I see it.

Sounds like a not-so-veiled passive-aggressive threat, the kind of stuff that makes a working environment toxic. Code reviews should be a moment of collective growth, not the excuse to jump on a pedestal and proclaim some sort of absolute revealed truth.

@kcat
Copy link

kcat commented Feb 1, 2025

As I said, I respectfully disagree and see case ranges much more harmful because they introduce a dependency on the order of the enum values.

It would only be an issue when also paired with a default: case, wouldn't it? If you have an enum like

enum my_enum { a, b, c, d, e, f };

// ...

switch(get_my_enum())
{
    case a:
        return do_a();
    case b:
        return do_b();
    case c ... f:
        return do_other();
}

Then you change the enum to move around the values like:

enum my_enum { a, b, d, c, f, e };

you now get warned that d and e aren't handled in the switch (are they meant to be part of the c...f "group" still? is their new location meant to indicate they may need different handling now?). Similarly, if you add new enum values

enum my_enum { a, b, c, d, e, f, g, h };

you get warned that g and h are unhandled. In both these cases, you now know you should either add the necessary cases, or update the case range for the new/updated values. If you add something within the range, like c1, d2, etc, yeah the range case will take them, but so would a default case so it's no better or worse in that case. In contrast, if you used default: instead of case c ... f:, you get no heads up that these switch statements should be looked at to ensure they're still behaving as intended with the changed enum.

@AndrewBloom
Copy link

Yes, you are right regarding those two cases, and seeing it written down, made me doubt about the case I was mostly worried about (the range swallowing those a and b cases). Truth is that I didn't know about this extension before, and so I decided to test it on gcc 14.2 on one of these online website, stepping into a tester's shoes and trying some edge cases. It's true that the compiler emits an error if there are duplicated cases, or overlapping ranges. So, situations like these are prevented:

enum {a, b, d, c} my_enum;  // was ordered alphabetically before being reordered
...
case a ... c: /* some actions */; break;
case d: /* other actions */; break;  // <-- ERROR, now overlaps with previous range

on the other end, something like that is possible (elements of groups inadvertently swapped):

enum {a, b, f, d, e, c, g} my enum; // was ordered alphabetically before being reordered
...
case a: * some actions */; break;
case b ... d: /* some other actions */; break;
case e ... g:  /* some other actions */; break;

another case that could generate confusion would be something like (groups are based on the integers values):

enum T {a=0x0, b1=0x1, b2=0x1<<4, b3=0x1<<1, c1=0x1<<2, c2=0x1<<3, d1=0x1<<6};  // b2 is added in a second moment using the first bit free (?!)
...
volatile T j = b2;

switch (j) { // prints Branch C & D (!!)
case a: std::cout << "Branch A" << std::endl; break;
case b1 ... b3: std::cout << "Branch B" << std::endl; break;
case c1 ... d1: std::cout << "Branch C & D" << std::endl; break;

I totally agree these corner cases are like playing devils' advocate and the ranges are safer than what I originally thought.

I'm wondering, is there any language that has no equivalent of default:?

@jwakely
Copy link
Contributor

jwakely commented Feb 4, 2025

enum T {a=0x0, b1=0x1, b2=0x1<<4, b3=0x1<<1, c1=0x1<<2, c2=0x1<<3, d1=0x1<<6};  // b2 is added in a second moment using the first bit free (?!)
...
volatile T j = b2;

switch (j) { // prints Branch C & D (!!)
case a: std::cout << "Branch A" << std::endl; break;
case b1 ... b3: std::cout << "Branch B" << std::endl; break;
case c1 ... d1: std::cout << "Branch C & D" << std::endl; break;

So you just wouldn't use ranges for flag enums used as a bitset, where the specific values don't matter and they're just distinct bits that can be tested for being 0 or 1.

Enums have many different uses, and not everybody uses them the same way, so not every feature needs to work with every use case. Just as I said at #2163 (comment)

The devil should get better legal advice ;-)

@kcat
Copy link

kcat commented Feb 4, 2025

another case that could generate confusion would be something like (groups are based on the integers values):

I would support a guideline suggesting enum values should be defined in ascending numeric order (where the value of any given enumeration, aside from the first, is equal to or greater than the previous). That would also take care of cases like

if(j >= c1) // True if j = b2

where a switch can't work for some reason. Currently clang-tidy has the readability-enum-initial-value check which says either all or no enum values should be explicitly set, with the exception that the first can have an explicit value and all remaining are implicit. E.g.

enum A { // okay
  a,
  b,
  c,
  d,
};

enum B { // okay
  a = 3,
  b,
  c,
  d,
};

enum C { // okay
  a = 1,
  b = 10,
  c = 12,
  d = 20,
};

enum D { // bad
  a,
  b = 2,
  c = 5,
  d = 7,
};

enum E { // bad
  a = 4,
  b,
  c = 3,
  d,
};

This is to help ensure an implicitly assigned value doesn't unexpectedly equal another (e.g., E::a == E::d, which may not be apparent at a glance). Extending a rule like this to say each subsequent value after the first should be equal to or greater than the previous would make sense to help catch surprises in relative ordering, beyond implicitly overlapping values, handling issues like in your last example where the numeric order being different from the defined order isn't immediately obvious.

@jwakely
Copy link
Contributor

jwakely commented Feb 4, 2025

That would complain about the following, which seems like reasonable code to me (apart from poor naming because it's just an example), and perfectly safe:

enum Bits {
 flag1 = 1, flag2 = 2, flag3 = 4, flag4 = 8,
 combo1 = flag1|flag2,
 combo2 = flag3|flag4,
};

@andim2
Copy link

andim2 commented Mar 29, 2025

Quick clarification: I'm in the "terrible advice" camp.

And if you add another enumerator, this guideline says that a tool enforcing the guideline should complain.

  1. external tool (NOT the compiler, inherently - which that should have been, preferably, always) ===> NOT Fail-Fast
  2. it cannot (due to default: having been applied by the programmer, at the current protocol status of the enum directly/only at time of development -
    which is NOT the protocol status several years later, when a surprisingly large percentage of enums within the codebase will have been protocol-extended *))

*) this is all too common in our code, where enums ("enumeration"!!) frequently do get extended (device types, operation modes, ... **))
**) this in itself might be a strong code smell though (applying woefully per-device-specific manual separation as opposed to simply providing some generic-protocol-feature-related virtual)

then there's no need to add a default case.

But people will easily do it - considering current (read: "outdated"!) protocol situation only.

"Reason

Code clarity. Improved opportunities for error detection."

Not so. It works against "Improved opportunities for error detection.".
(the key aspect here is: compiler-side vs. developer-side)
A developer may detect an issue here.

  1. when having time
  2. when manually observing the entire enum switch set implementation [style] as provided by the author (which may or may not be in order, even! Or even legible...)

The compiler will detect an issue here (iff properly coded and warnings-configured!).

The actual issue here is
that the very moment that default: gets added issue detection will get shifted (NON-Shift-Left!) from compile-time to development-activity-time, else worst case, runtime (CORRUPTION-level-type bug situation...).

"a compiler might reasonably assume that you intended to handle all cases:"

"reasonably" - not so (extension of the enum protocol is a common situation, see above - thus "intended" could be considered an invalid concept - "intended when??")

My current practice is:

switch (tiny_enum)
{
  case a:
    ...
    break;
  case b:
    ...
    break;
  case c:
    // no handling needed (this case is not relevant in our enum consumption)
    break;
}

(full set specified - we'll Fail-Fast have compile-time warning[-as-error] complaint here if things turn abnormal)

switch (huge_enum)
{
  case a:
    ...
    break;
  case c:
    ...
    break;
  default:
    BOOST_ASSERT(false); // unhandled or corrupt case!?
    (NOTE: one could possibly argue that
    this should instead [better] be exceptional-error-based behaviour here; think unit testing ASSERT issue etc.)
    break;
}

(tiny subset specified - we'll have runtime-only complaint here, if:
a) the author missed one crucial case in its relevant subset of enums, initially
b) protocol changed (potentially incompatibly)
c) relevant case handler got removed
)

You've shown why you don't want to use default in your code. Fine. So don't use it.

AFAIUI, C++ Core Guidelines is to provide hot-path recommendations/usual practice in order to achieve properly Best Practice (robust/flexible/efficient/...) implementations.
I.e. it should pretty reliably show the general populace which way to best do things,
not resort to telling individual experts(!) that they may resort to not following the recommendation (...ouch).
This means that it should recommend (mention!) items which are justifiably Best Practice, and not items which are (as explained above) sufficiently sketchy.
"mention" meaning: if an item is sufficiently sketchy as to be very debatable, then it might be actually much better to not even have such a topic item (specific situation: ES.79),
at least as long as such an item does not manage to reliably explain the entire (contrived) issue situation. Which IMHO ES.79 really doesn't, yet.

The point of the C++ CoreGuidelines is to provide a set of good practices to minimize error, and to provide a set of rules that tooling can use to point out potential problems, all to prevent unintentional behavior aka bugs

Hah, and here @kcat even explained that very issue identically and fully independently from me. :-)

Another strong point is that
we cannot afford to lose out on Fail-Fast error detection precision -
An enum definition is central and will thus be centrally modified,
yet with potentially multi-dozen different consumer sites somewhere, all making use of it
(potentially even very foreign thus firmly decoupled since non-upstream consumer-site areas!).
Good luck trying to have ensured a reliable since Fail-Fast (Shift-Left) protocol consumption at all these sites, at all times, with a "nice" default: applied there.
Rather than applying default: (a completely behaviour-changing action), to express intent a simple comment such as // this switch is intended to handle all enum cases would be non-invasive.

lol, good one. Do you have any proper rationale for breaking millions of lines of working code?

Yeah, that argument was quite easy to disagree with ;)

(though of course there could be mild deprecation activities such as compile warning flagging -
thereby obeying the usual
normal operation - warning-level operation - error-level bailout (final removal!)
staged handling)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants