iamandi / nanopb

Automatically exported from code.google.com/p/nanopb
zlib License
0 stars 0 forks source link

Allow max_count and max_size field to be defined by enum instead of constant #98

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Describe the requested feature:

It would be useful to be able to define an enum field, such as "MAX_ARRAY_SIZE" 
and then use it in the nanopb annotation for max_size and max_count for 
repeated and string/byte array members:

enum MyConstants {
   ARRAY_LEN = 16;
}

message MyMessage {
   repeated MyOtherMessage member = 1 [(nanopb).max_count = ARRAY_LEN];
}

In what situation would the feature be useful:

Defining multiplicities as a numeric value leads to "magic numbers" creeping 
into the code.  If the numeric value needs to be used elsewhere, it has to be 
copied and maintained in two places, which is a safety and maintainability 
issue.  Using the enum method, there could be a single source for the array 
lengths.

Original issue reported on code.google.com by allen.m...@gmail.com on 30 Dec 2013 at 11:04

GoogleCodeExporter commented 9 years ago
Hmm yeah, I recognize the need. However, it's not going to be easy to implement.

The (nanopb).max_count field is defined as int32, and protoc won't accept an 
enum there.

I see a few options:

1) Try to submit a patch in Google's protoc that would allow enum values there. 
I'm not so sure that it would be accepted. Also it wouldn't really work for 
.options files, as the declarations would be in a different context.

2) Define another string field like "max_count_enum" that would take the name 
of the enum value as a string. Quite ugly IMO, and would require duplicating 
each option type.

3) Modify the generator so that the field max_count and max_size values are 
available as #defines. But this would still be a bit cumbersome if you want 
multiple fields to always have the same size (though the wildcards in .options 
do help).

None of these seems particularly good, though perhaps 3) is the most feasible.

Original comment by Petteri.Aimonen on 31 Dec 2013 at 9:04

GoogleCodeExporter commented 9 years ago
Another option might be to somehow pre-process to .proto file before passing it 
to the protoc compiler.  If you start with:

enum MyConstants {
   ARRAY_LEN = 16;
}

message MyMessage {
   repeated MyOtherMessage member = 1 [(nanopb).max_count = ARRAY_LEN];
}

You could process the file to translate ARRAY_LEN to 16 before delivering it to 
protoc.

It's a bit hacky but I think more maintainable than the other options.

I would also be fairly happy with option number 2 :)  I think that will be the 
most maintainable for users of nanopb, though probably the most awkward for 
implementation in nanopb.

Original comment by allen.m...@gmail.com on 7 Jan 2014 at 7:07

GoogleCodeExporter commented 9 years ago
For preprocessing, one could probably just use cpp. I.e. something like:

#define ARRAY_LEN_C 16

enum MyConstants {
    ARRAY_LEN = ARRAY_LEN_C;
}

message MyMessage {
    repeated MyOtherMessage member = 1 [(nanopb).max_count = ARRAY_LEN_C];
}

Which wouldn't require any change in nanopb.

But all of these are quite hackish, so unless a better idea comes along I don't 
think it is worth an implementation.

Original comment by Petteri.Aimonen on 7 Jan 2014 at 7:27

GoogleCodeExporter commented 9 years ago
I'm not sure I understand the last method, with the #define?  Where would the 
#define go?

Original comment by allen.m...@gmail.com on 9 Jan 2014 at 7:06

GoogleCodeExporter commented 9 years ago
No good fix discovered.

I recommend the following approach:
- Use options file. That way all the sizes are in one place neatly.
- And use sizeof() or pb_membersize() in C to get the sizes.

Original comment by Petteri.Aimonen on 2 Apr 2014 at 6:24