Disambiguate generated properties in C# #10269
Conversation
| if (property_name == descriptor->containing_type()->name() | ||
| || property_name == "Types" | ||
| || property_name == "Descriptor") { | ||
| || property_name == "Descriptor" |
There was a problem hiding this comment.
Maybe just add everything from System.Object, like GetType, MemberwiseClone I think, etc.
There was a problem hiding this comment.
Yes, I forgot GetType and MemberwiseClone for some reason. Will add those tomorrow when I move to a set-based approach.
There was a problem hiding this comment.
It turns out GetType and MemberwiseClone are slightly trickier - they currently only generate warnings, not errors, so adding an underscore could be a breaking change in that case. I'm not sure of the best solution for that at the moment. (I'm tempted to go really cautiously here. We haven't heard any reports of those causing problems, so I think I'd prefer to leave them for now.)
|
Changes since previous review:
|
| @@ -378,16 +378,32 @@ std::string GetFieldConstantName(const FieldDescriptor* field) { | |||
| return GetPropertyName(field) + "FieldNumber"; | |||
| } | |||
|
|
|||
| // Names of members declared or overridden in the message. | |||
| // Used in GetPropertyName below. | |||
| static const auto& reserved_member_names = *new std::unordered_set<std::string>({ | |||
There was a problem hiding this comment.
unfortunately, you are doing this at the global scope which leads to issues. You need to do this at function scope
There was a problem hiding this comment.
Fixed, I think - but please check carefully! I'm hoping that by declaring it with the static modifier it's still only going to be initialized on first use rather than on every call, right?
|
There appear to be a lot of failing checks. Can you rebase onto head? |
|
Rebased now, but I suspect that wasn't the problem. We'll see what happens... |
|
I agree, but one can hope :) |
|
Looks like it's now just "all the MacOS tests" that are failing - I don't know enough about MacOS to help on that front :( |
This won't remove all possibilities of naming collisions, but will address the simplest ones. The "test" is just to add all the reserved names in a proto file: if the generated code builds, it works. Note that before this change, using any of these field names would result in a compile-time error, so this is not a breaking change. Generated code is in the next commit. Fixes protocolbuffers#8810
|
@fowles: All green now - please merge if you're happy to do so :) |
No description provided.