Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EmbeddedProto v3.5.0 - build warnings using gcc #77

Open
eS-Ha-79 opened this issue Jun 19, 2024 · 5 comments
Open

EmbeddedProto v3.5.0 - build warnings using gcc #77

eS-Ha-79 opened this issue Jun 19, 2024 · 5 comments

Comments

@eS-Ha-79
Copy link

Hi,

i'm using gcc (13.2.1 but shouldn't matter) with additional warnings enabled:
-Wall -Wextra -Wdouble-promotion

float fields

With messages containing float fields, the following warning(s) are produced in the serialize() function:

warning: implicit conversion from 'float' to 'double' to match other operand of binary expression [-Wdouble-promotion]
  910 |       if((0.0 != minimum_.get()) && (::EmbeddedProto::Error::NO_ERRORS == return_value))

I guess changing "0.0" to "0.0f" in Field.py -> class FieldBasic(Field) -> type_to_default_value (lines 159+) for
the dictionary entry FieldDescriptorProto.TYPE_FLOAT should be enough.

empty messages

Using empty messages (like google.protobuf.Empty) there are some unused parameter warnings (generated constructors, assignment operators and serialize() ). Personally, i added [[maybe_unused]] attributes in TypeDefMsg.h but there might be better ways, since this is CPP17+:

  • Line 38: {{ typedef.get_name() }}([[maybe_unused]] const {{typedef.get_name()}}& rhs )
  • Line 60: {{ typedef.get_name() }}([[maybe_unused]] const {{typedef.get_name()}}&& rhs ) noexcept
  • Line 100: {{ typedef.name }}& operator=([[maybe_unused]] const {{ typedef.name }}& rhs)
  • Line 123: {{ typedef.name }}& operator=([[maybe_unused]] const {{ typedef.name }}&& rhs) noexcept
  • Line 159: ::EmbeddedProto::Error serialize([[maybe_unused]] ::EmbeddedProto::WriteBufferInterface& buffer) const override
@BartHertog
Copy link
Contributor

Hello @eS-Ha-79,

Thank you for writing in with these issues! Your input helps improve embedded proto.

float fields: This has been added to the backlog. It is easy to add and will indeed clarify the meaning of the code.

empty messages: CPP17, at the moment, is not the default. There are still customers with projects on CPP14. We may increase the requirement to 17 with the next major release of EmbeddedProto. If that is the case we will include your suggestion.

On a side note, may I ask what your use case is for empty messages?

Best regards,

Bart

@eS-Ha-79
Copy link
Author

Hey Bart,

was playing around with some sort of lightweight/naive (whatever one may call it ;-) ) RPC and used empty messages for commands w/o parameters, e.g.

message request {
  int32 request_number = 1;
  oneof group {
    float set_motor_speed = 2;
    google.protobuf.Empty get_motor_speed = 3;
  }
}

and

message response {
  int32 to_request_number = 1;
  oneof group {

     float current_speed = 3;
  }
  optional int32 error = 4;
}

Best regards,
Soeren

@BartHertog
Copy link
Contributor

Hi,
That is an interesting way to think of empty messages. Using the oneof with the empty messages is interesting. I understand that you than want the message to be very light.

I was used to using enums for commands thus far.

Best regards,

Bart

@eS-Ha-79
Copy link
Author

Hey,

yes keeping it small was the idea.

Regarding the floats 0.0 vs 0.0f:
gcc and clang both warn about the "double promotion" - BUT gcc produces the same output for both versions whilst clangs output differs...
See: compiler explorer

Best regards,
Soeren

@BartHertog
Copy link
Contributor

Hi @eS-Ha-79,

Both the float default value and a fix for the unused parameter have been implemented in the upcoming v4 release of Embedded Proto.

For the unused parameter, I went the old (void)xxx route. The [[maybe_unused]] attribute is part of the function signature. If I had used it, it needed to be in the base classes as well. It is now more readable and will only show up in empty messages.

Currently, it is only visible in the develop-v4 branch. This is an active development branch and is not suitable for usage yet.

Best regards,

Bart

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

No branches or pull requests

2 participants