On Wed, Aug 04 2021, Josh Steadmon wrote:
> Add a function to get human-readable names for the various wire protocol
> versions.
>
> Signed-off-by: Josh Steadmon <steadmon@xxxxxxxxxx>
> ---
> protocol.c | 14 ++++++++++++++
> protocol.h | 6 ++++++
> 2 files changed, 20 insertions(+)
>
> diff --git a/protocol.c b/protocol.c
> index 052d7edbb9..7ec7ce896e 100644
> --- a/protocol.c
> +++ b/protocol.c
> @@ -14,6 +14,20 @@ static enum protocol_version parse_protocol_version(const char *value)
> return protocol_unknown_version;
> }
>
> +const char *format_protocol_version(enum protocol_version version)
> +{
> + switch (version) {
> + case protocol_v0:
Don't indent "case" one more than "switch".
(Looking at CodingGuidelines we only have that advice for *.sh, but we
follow it for C too).
> + return "0";
> + case protocol_v1:
> + return "1";
> + case protocol_v2:
> + return "2";
> + default:
> + return "UNKNOWN_VERSION";
Using the "default" case like that is an anti-pattern, i.e. you
enumerted all arms except protocol_unknown_version, which is implicitly
covered by your "default" case.
Just list it, and don't have a "default" case, then the compiler will
complain if we ever add a new case that's not covered.
As an aside, and not strictly needed here: More generally between this
and parse_protocol_version() it seems like this would benefit from just
declaring the bidirectional mapping beween the enum fields and string
values, and then have this and that function use that. See
e.g. object_type_strings and associated functions in object.c for
another enum that has such a bidirectional lookup.
