Skip to content

Exhaustive enum switch#7701

Draft
cjen1-msft wants to merge 6 commits intomicrosoft:mainfrom
cjen1-msft:tired-switching
Draft

Exhaustive enum switch#7701
cjen1-msft wants to merge 6 commits intomicrosoft:mainfrom
cjen1-msft:tired-switching

Conversation

@cjen1-msft
Copy link
Contributor

This adds -Wswitch-enum which requires a branch for every type of an enum.
And it also adds -Wcovered-switch-default which throws an error if every case of an enum is covered.

And then fixes all of the switch statements.

case CurveID::NONE:
case CurveID::CURVE25519:
case CurveID::X25519:
throw std::logic_error(fmt::format("Unknown curve {}", curve_id));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw std::logic_error(fmt::format("Unknown curve {}", curve_id));
throw std::logic_error(fmt::format("Unsupported curve {}", curve_id));

switch (jwk_curve)
{
case JsonWebKeyECCurve::P521:
throw std::logic_error(fmt::format("Unknown JWK curve {}", jwk_curve));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw std::logic_error(fmt::format("Unknown JWK curve {}", jwk_curve));
throw std::logic_error(fmt::format("Unsupported JWK curve {}", jwk_curve));

case CurveID::NONE:
case CurveID::SECP384R1:
case CurveID::SECP256R1:
throw std::logic_error(fmt::format("Unknown EdDSA curve {}", curve_id));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are ECDSA, not EdDSA, so ideally we should find a way to validate them separately. The message would be something like "Not a valid EdDSA curve then".

}
}
return format_to(ctx.out(), "{}", s);
throw std::logic_error("Unhandled value for ForwardingRequired");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw std::logic_error("Unhandled value for ForwardingRequired");
throw std::logic_error(fmt::format("Unhandled value for ForwardingRequired: {}", v));

Slightly more debuggable.

"Unsupported SEV-SNP product for TCB version policy");
}

throw std::logic_error("Unsupported SEV-SNP product for TCB version policy");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw std::logic_error("Unsupported SEV-SNP product for TCB version policy");
throw std::logic_error(fmt::format("Unsupported SEV-SNP product for TCB version policy: {}", product));

@@ -582,11 +576,6 @@ pRb21iI1NlNCfOGUPIhVpWECAwEAAQ==
max_client_response_size));
break;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused as to why there is no default here, but we have it in other places (like to_string(Product) below) where we are ostensibly exhaustive.

throw std::logic_error("Unknown SEV-SNP product");
}

throw std::logic_error("Unknown SEV-SNP product");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw std::logic_error("Unknown SEV-SNP product");
throw std::logic_error(fmt::format("Unknown SEV-SNP product: {}", product));

Comment on lines +282 to +283
throw std::logic_error(
"Unsupported SEV-SNP product for TCB version policy");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this supposed to be unreachable? I understand a default-throw, that's "this might currently be unreachable, but if we make it reachable in future it throws". But this trailing throw, if we now think we have compile-time enforcement that it remains unreachable? I think that should be std::unreachable() (or nonstd::unreachable() calling __builtin_unreachable(), until we have C++23 support).

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

Successfully merging this pull request may close these issues.

3 participants