Avoid C++11 warning about switch on proto3 enum w/o default clause?

[chh...@mesosphere.io]

Hi all,

I'm from the Apache Mesos community and we're working on things that uses proto3.
We usually turn on compiler options to report missing values in switch statements,
and don't do a "default" clause. This gives us the benefit of capturing missing enum
values during compile time. However, it seems that for proto3's open enum values,
we need to either manually adding those `*_INT_MAX_SENTINEL_DO_NOT_USE_` values
in switch statements, or add a default clause to avoid such errors but then we lose
the benefit I mentioned above. It seems that the compiler is not clever enough
to infer that clauses for those sentinel values are not needed for switch statements
that are inside `if(Enum_IsValid(...))` statements.

My question is, what's the best practice if I don't want to add a default clause?
Is it safe to add either `*_INT_MAX_SENTINEL_DO_NOT_USE_` symbols
or `google::protobuf::kint32max` into switch statements, such that these will
remain unchanged in future versions of protobuf library? Or is there a better way
to address this?

Thanks,
Chun-Hung

[Feng Xiao]

The recommendation is to always have a default case when handling proto3 enums. Unlike regular enums that fall in a defined list of possible values, proto3 enums can have unrecognized values when a client receives data from a remote server with a new enum definition. To ensure such unrecognized values are correctly handled, a default case must be added.

For example, suppose you have a client built with such a proto:

message Foo {

  enum Type {

    FOO = 0;

    BAR = 1;

  }

  Type type = 1;

}

and later someone adds a new value into the enum and uses the new definition on the server side:

message Foo {

  enum Type {

    FOO = 0;

    BAR = 1;

    BAZ = 2;

  }

  Type type = 1;

}

If your client receives data from the server with type = BAZ, foo.type() will return 2, which you can't catch with any predefined values. If you don't have default case, undefined behavior ensures.

// Wrong version of client-side code:

int HandleFoo(Foo foo) { 

  switch (foo.type()) {

    case FOO: return ...;

    case BAR: return ...;

    case *_INT_MAX_SENTINEL_DO_NOT_USE: return ...;

  }

  // You can never get here, or maybe you can?

}

// The correct version

int HandleFoo(Foo foo) { 

  switch (foo.type()) {

    case FOO: return ...;

    case BAR: return ...;

    default: return ...;  // There is an enum value we don't know about, handle it appropriately.

  }

  // You can never get here.

}

 

Thanks,
Chun-Hung

--
You received this message because you are subscribed to the Google Groups "Protocol Buffers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to protobuf+unsubscribe@googlegroups.com.
To post to this group, send email to prot...@googlegroups.com.
Visit this group at https://groups.google.com/group/protobuf.
For more options, visit https://groups.google.com/d/optout.

[chh...@mesosphere.io]

Hi Feng,

Thanks for answering.

As suggested by the official documentation:

https://developers.google.com/protocol-buffers/docs/reference/cpp-generated#enum

We can handle the unknown enum value through either a `default` clause or an `if` check as follows:

// Wrong version of client-side code:

int HandleFoo(Foo foo) {

  if (Foo::Type_IsValid(foo.type()) {

    switch (foo.type()) {

      case FOO: return ...;

      case BAR: return ...;

      case google::protobuf::kint32min:

      case google::protobuf::kint32max:

        // UNREACHABLE.

    }

  } else {

    // Handle the unknown enum value properly.

  }

}

So this way we can ensure that the unknown enum values are handled,

and also the compiler can check if an enum value is missing in the `switch` statement for us.

Is it future-proof to use `google::protobuf::kint32max` or `*_INT_MAX_SENTINEL_DO_NOT_USE_`?

It seems to me that there's some merit to formalize this in the proto3 language. Thoughts?

Best,

Chun-Hung

Thanks,
Chun-Hung

To unsubscribe from this group and stop receiving emails from it, send an email to protobuf+u...@googlegroups.com.

[Feng Xiao]

On Mon, Oct 30, 2017 at 6:19 PM, <chh...@mesosphere.io> wrote:

Hi Feng,

Thanks for answering.

As suggested by the official documentation:

https://developers.google.com/protocol-buffers/docs/reference/cpp-generated#enum

We can handle the unknown enum value through either a `default` clause or an `if` check as follows:

// Wrong version of client-side code:

int HandleFoo(Foo foo) {

  if (Foo::Type_IsValid(foo.type()) {

    switch (foo.type()) {

      case FOO: return ...;

      case BAR: return ...;

      case google::protobuf::kint32min:

      case google::protobuf::kint32max:

        // UNREACHABLE.

    }

  } else {

    // Handle the unknown enum value properly.

  }

}

So this way we can ensure that the unknown enum values are handled,

and also the compiler can check if an enum value is missing in the `switch` statement for us.

Is it future-proof to use `google::protobuf::kint32max` or `*_INT_MAX_SENTINEL_DO_NOT_USE_`?

No, it will break right away when we add another "*_XXX_SENTINEL_DO_NOT_USE_", or when we want to rename "XXX_DO_NOT_USE" to "XXX_DO_NOT_USE_PLEASE_ADD_DEFAULT_CASE".

 

To unsubscribe from this group and stop receiving emails from it, send an email to protobuf+unsubscribe@googlegroups.com.