C++ enum codegen mixes int and uint64

[uecasm]

What version of protobuf and what language are you using? Version: 3.11.4 Language: C++

What operating system (Linux, Windows, ...) and version? Windows 10

What runtime / compiler are you using (e.g., python version or gcc version) VS2017 15.9.19

What did you do?

syntax = "proto2";

message StatusMessage {
    enum ConnectionState {
        CONNECTED = 1;
        DISCONNECTED = 2;
        UNCONFIGURED = 3;
    }
    optional ConnectionState connection_state = 1;
}

What did you expect to see No compiler warnings.

What did you see instead? In the generated StatusMessage::_InternalParse method for this field, it produces this code:

::PROTOBUF_NAMESPACE_ID::uint64 val = ::PROTOBUF_NAMESPACE_ID::internal::ReadVarint(&ptr);
CHK_(ptr);
if (PROTOBUF_PREDICT_TRUE(::StatusMessage_ConnectionState_IsValid(val))) {

Note that val is declared as a uint64 -- but StatusMessage_ConnectionState_IsValid accepts only an int parameter.

bool StatusMessage_ConnectionState_IsValid(int value) {

This generates the narrowing-conversion warning C4244 (which breaks my build since I have warnings-as-errors enabled).

This issue does not occur in Protobuf 3.6.1.

(Also, FWIW, there appear to be new C4127 warnings in the generated code as well (which are less serious than the above). I've worked around that by disabling them, but it would be nice if the code could be warning-clean.)

[uecasm]

This is possibly a separate issue, but apparently ByteSize is now deprecated in favour of ByteSizeLong, but the latter returns a size_t whereas all the other message API (eg. SerializeToArray) continues to use int. This seems inconsistent.

[ssainz]

Also experiencing warnings like the ones described in this issue.

warning C4244: 'argument': conversion from 'uint64_t' to 'int', possible loss of data

[ssainz]

Hi, is there an update on this one?

[fowles]

No one is working on this at the moment.

[tomthomson]

I was facing the same narrowing compiler warning when I tried to upgrade protobuf from 3.8 to the latest version 3.21.6. I was using gcc-11 and clang-14 and the compile option -Wconversion

I noticed https://github.com/protocolbuffers/protobuf/pull/6768 which tackled the narrowing conversion warning for Windows.

I went on to investigate this and came up with below patch. This solved the conversion warnings for me with compilers gcc-11 and clang-14.

Index: src/google/protobuf/compiler/cpp/parse_function_generator.cc
===================================================================
diff --git a/src/google/protobuf/compiler/cpp/parse_function_generator.cc b/src/google/protobuf/compiler/cpp/parse_function_generator.cc
--- a/src/google/protobuf/compiler/cpp/parse_function_generator.cc	(revision 24487dd1045c7f3d64a21f38a3f0c06cc4cf2edb)
+++ b/src/google/protobuf/compiler/cpp/parse_function_generator.cc	(date 1663937080247)
@@ -1371,7 +1371,7 @@
         format.Set("enum_type",
                    QualifiedClassName(field->enum_type(), options_));
         format(
-            "$uint64$ val = ::$proto_ns$::internal::ReadVarint64(&ptr);\n"
+            "$uint32$ val = ::$proto_ns$::internal::ReadVarint32(&ptr);\n"
             "CHK_(ptr);\n");
         if (!HasPreservingUnknownEnumSemantics(field)) {
           format("if (PROTOBUF_PREDICT_TRUE($enum_type$_IsValid(val))) {\n");

[ssainz]

Awesome fix @tomthomson ! As I was preparing PR with your fix noticed it just got picked up by https://github.com/protocolbuffers/protobuf/pull/10353. Thus, this should be fixed once such PR finds its way to a branch that gets released.

[uecasm]

The caveat with both of those is that they will probably break if someone uses enum values in the 64-bit range (which would be unusual, but I think currently not illegal). Perhaps that would need a proto compiler error diagnostic, or an alternative fix.

[milahu]

they will probably break if someone uses enum values in the 64-bit range

make the size explicit? enum32 vs enum64

@tomthomson also the isValid function should be fixed, from int to uint32

example from ricochet: FileChannel.proto

enum FileTransferResult {
    Success = 0;
    Failure = 1;
    Cancelled = 2;
}

FileChannel.pb.cc

-bool FileTransferResult_IsValid(int value) {
+bool FileTransferResult_IsValid(uint32 value) {
   switch (value) {
     case 0:
     case 1:
     case 2:
       return true;
     default:
       return false;
   }
 }

[github-actions[bot]]

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago.

[github-actions[bot]]

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please reopen it.

This issue was closed and archived because there has been no new activity in the 14 days since the inactive label was added.