Skip to content

build: Fix compilation errors when building with Protobuf 29.0+ #371

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ManManson
Copy link

@ManManson ManManson commented Apr 6, 2025

Starting from Protobuf 29.0, protobuf::Message::GetTypeName() can return either std::string or absl::string_view, depending on Abseil build options.

For more details, see:

https://github.com/protocolbuffers/protobuf/commit/e13b8e999b3922d0633802c7f90e39af50a31d76

Detect the return type of the method in compile-time and construct a supplementary std::string, from which we can obtain a proper null-terminated buffer and pass it to the internal assertion function, if needed.

Fixes: #370

@chriku
Copy link

chriku commented Apr 11, 2025

string_view::data (std, i assume abseil is the same) does not necessarily return zero-terminated strings, which are required by %s

@ManManson
Copy link
Author

Nice catch! Indeed, it cannot be used this way. Abseil's string_view is exactly the same in this regard. I'll need to rewrite the code to automatically detect the return type of GetTypeName() and construct a null-terminated buffer out of it, if needed.

@ManManson ManManson force-pushed the protobuf_compilation_fix_post_29_0_version branch from 0600204 to 18da6a3 Compare April 11, 2025 20:47
@ManManson
Copy link
Author

Force-pushed the branch to fix the problem discovered by @chriku. SteamNetworkingIdentityToProtobuf macro will attempt to perform a compile-time check to see if msg.GetTypeName() returns std::string or not (in this case, any StringViewLike type will do).

This fix uses direct if constexpr + std::is_same_v combination for comparison instead of relying on various protobuf-related macros since it's both easier to read this way and, AFAIU, the macros defined inside the protobuf aren't exposed to the callers of the library.

@chriku
Copy link

chriku commented Apr 13, 2025

This seems to be a bit much overkill for an error path that terminates the program anyway. if I'd gotten a (std::, I assume abseil just works similarly) string_view or sometimes string, I would have just have yeeted it into the std::string constructor anyway, which has the nice c_str, making the expression something like: %s", std::string(msg.GetTypeName()).c_str().

@ManManson
Copy link
Author

@chriku Sure, you're absolutely right. This is an exceptional path, so indeed there's no need for extra typing to avoid a string copy. I'll post a fix a bit later. Thanks for the review!

Starting from Protobuf 29.0, `protobuf::Message::GetTypeName()` can
return either `std::string` or `absl::string_view`, depending on
Abseil build options.

For more details, see:

	protocolbuffers/protobuf@e13b8e9

Construct a temporary `std::string` to obtain a proper null-terminated
buffer to use in `AssertMsg*`.
Note that the lifetime of this instance will end just after the
`AssertMsg2()` function call returns, hence this usage is safe (see
https://eel.is/c++draft/class.temporary#4 for more details).

Fixes: ValveSoftware#370

Signed-off-by: Pavel Solodovnikov <[email protected]>
@ManManson ManManson force-pushed the protobuf_compilation_fix_post_29_0_version branch from 18da6a3 to f938f6f Compare April 15, 2025 21:15
@ManManson
Copy link
Author

Force-pushed the branch to implement the suggestion from @chriku to simplify the code. Also added a brief explanation about code safety with respect to the latest C++ standard draft.

@chriku
Copy link

chriku commented Apr 16, 2025

c++ lifetimes are always funny, eh? but the new code seems appropriate for the situation.

PS: A small (hopefully helpful) hint: You (@ManManson) sound strangely AI like, be careful that you don't get misidentified and your PRs closed in fear of copyright uncertainties or worse

@ManManson
Copy link
Author

Yep. That's the reason I've provided a thorough comment so that it doesn't make anyone looking at the code to raise an eyebrow.

I think I've been writing like that long before AIs became manistream. I just happen to like explicit and verbose communication style when participating in public projects. 😄

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

Successfully merging this pull request may close these issues.

(Linux) Fails to compile on latest version of protobuf
2 participants