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

Already on GitHub? Sign in to your account

Fixes warning: default label in switch which covers all enumeration valu... #771

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
Contributor

orj commented Feb 1, 2013

...es [-Wcovered-switch-default].

Also simplifies the code. Less lines and ensures there is always a return value from the method. No need for switch or default case.

You didn't put the pragma around all the cases you reverted Matt. So here is another little patch that I think makes the code better and fixes the warning.

Fixes warning: default label in switch which covers all enumeration v…
…alues [-Wcovered-switch-default].

Also simplifies the code.  Less lines and ensures there is always a return value from the method.  No need for switch or default case.
Contributor

mattt commented Feb 1, 2013

You didn't put the pragma around all the cases you reverted.

How do you mean, exactly?

Also, while this fix is undeniably less and simpler code—something I'm a fan of—it does so by breaking the contract of enumerated values. Although enums map directly to integer values, they are not semantically equivalent (beyond perhaps comparing a value to 0). Even though you and I know what that those enum values are, and that they almost certainly won't change in Foundation, cutting the corners here opens the code up to breaking behavior if that ever happens.

Contributor

orj commented Feb 3, 2013

Well you put the pragma for disabling the warning around one of the switches with a covered default: but not the other one that this patch changes.

As for the proposed patch, personally I think it is more future proof than the previous code. It is extremely unlikely that Apple will introduce a new NSStreamStatus value as it would break binary backwards compatibility and comparing enum values like this is quite common place. It is also faster code than using a switch or more complex if statement.

But you could also do this if you preferred and keep the semantics and not require a default: case in a switch that contains all the enum values (which is what is causing the warning):

NSStreamStatus status = self.inputStream.streamStatus;
return (status == NSStreamStatusNotOpen ||
status == NSStreamStatusOpening ||
status == NSStreamStatusReading ||
status == NSStreamStatusWriting);

On 02/02/2013, at 9:30 AM, Mattt Thompson notifications@github.com wrote:

You didn't put the pragma around all the cases you reverted.

How do you mean, exactly?

Also, while this fix is undeniably less and simpler code—something I'm a fan of—it does so by breaking the contract of enumerated values. Although enums map directly to integer values, they are not semantically equivalent (beyond perhaps comparing a value to 0). Even though you and I know what that those enum values are, and that they almost certainly won't change in Foundation, cutting the corners here opens the code up to breaking behavior if that ever happens.


Reply to this email directly or view it on GitHub.

Contributor

mattt commented Feb 3, 2013

Well you put the pragma for disabling the warning around one of the switches with a covered default: but not the other one that this patch changes.

Ah, I'd mixed up the two commits. fa3a775 fixes that.

I'm fine with the existing semantics, exhaustive and unoptimized as they are. In the case of documenting the state behavior in code like that, I don't mind being explicit for the sake of clarity.

Thanks again for your help, @orj.

@mattt mattt closed this Feb 3, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment