protobuf icon indicating copy to clipboard operation
protobuf copied to clipboard

Fix deprecated usage of `MutableRepeatedPtrField` by implementing iterator for `MutableRepeatedFieldRef`

Open tempoz opened this issue 1 year ago • 2 comments

Couldn't replace the deprecated usage of MutableRepeatedPtrField in src/google/protobuf/reflection_visit_field_info.h with MutableRepeatedFieldRef as the warning suggested because it did not implemet an iterator, so I implemented the iterator.

tempoz avatar Sep 11 '24 14:09 tempoz

There are some tests failing, but at an initial glance I suspect these are unrelated to the current change. Can you try rebasing?

zhangskz avatar Oct 14 '24 16:10 zhangskz

Done!

tempoz avatar Oct 16 '24 22:10 tempoz

Rebased it again, since it's been a while. Feel free to take another look; I can't re-run the tests myself.

tempoz avatar Nov 15 '24 17:11 tempoz

@tempoz Thank you for the pull request. I looked at this yesterday and unfortunately there is a big blocker preventing us from merging this, which is that in our internal code we support repeated strings of type absl::Cord. We have open sourced partial support for Cords but only for singular bytes fields. We also support repeated StringPiece fields--this is a weird legacy type that is similar to std::string_view but also supports aliasing to reduce copying.

These special types are tricky to handle, because while const iteration treats them as if they were plain string fields, for mutable iteration we would need to expose the actual type (e.g. absl::Cord). I think in principle we could figure out a way to make this work, but it would probably be a big project.

Unfortunately I think the best we could do for now would probably be to silence the deprecation warnings by just updating the code in reflection_visit_field_info.h to call MutableRepeatedFieldInternal and MutableRepeatedPtrFieldInternal, which is what the deprecated methods call internally.

acozzette avatar Nov 22 '24 19:11 acozzette

I updated this code in https://github.com/protocolbuffers/protobuf/commit/07faf360a3cdfcce79be26d6ed7561195f759f81 to work around the deprecated methods, so this will at least silence the compiler warnings. I'm going to close this pull request because unfortunately I don't think there is a path to merging it for the reasons I mentioned in my previous comment. Sorry for the bad news and thank you for the pull request nonetheless.

acozzette avatar Dec 02 '24 18:12 acozzette