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

Request: change std::optional to absl::optional #184

Open
translunar opened this issue Dec 11, 2024 · 3 comments
Open

Request: change std::optional to absl::optional #184

translunar opened this issue Dec 11, 2024 · 3 comments
Assignees
Labels
bug Something isn't working

Comments

@translunar
Copy link

This repo uses a lot of other absl headers, but it uses the std::optional header. This is the sole thing that necessitates C++17. If these types are changed to be absl::optional, C++17 is no longer required, and C++14 is sufficient.

@translunar
Copy link
Author

translunar commented Dec 11, 2024

Here's a diff I made for this. Sorry, no fork access at work.

diff --git a/pybind11_protobuf/check_unknown_fields.cc b/pybind11_protobuf/check_unknown_fields.cc
index 0e1e9ea..38ae96e 100644
--- a/pybind11_protobuf/check_unknown_fields.cc
+++ b/pybind11_protobuf/check_unknown_fields.cc
@@ -12,6 +12,7 @@
 #include "absl/strings/str_join.h"
 #include "absl/strings/string_view.h"
 #include "absl/synchronization/mutex.h"
+#include "absl/types/optional.h"
 #include "google/protobuf/descriptor.h"
 #include "google/protobuf/message.h"
 #include "google/protobuf/unknown_field_set.h"
@@ -181,7 +182,7 @@ void AllowUnknownFieldsFor(absl::string_view top_message_descriptor_full_name,
                                           unknown_field_parent_message_fqn));
 }
 
-std::optional<std::string> CheckRecursively(
+absl::optional<std::string> CheckRecursively(
     const ::google::protobuf::python::PyProto_API* py_proto_api,
     const ::google::protobuf::Message* message) {
   const auto* root_descriptor = message->GetDescriptor();
diff --git a/pybind11_protobuf/check_unknown_fields.h b/pybind11_protobuf/check_unknown_fields.h
index a448f83..26ea7ab 100644
--- a/pybind11_protobuf/check_unknown_fields.h
+++ b/pybind11_protobuf/check_unknown_fields.h
@@ -1,10 +1,10 @@
 #ifndef PYBIND11_PROTOBUF_CHECK_UNKNOWN_FIELDS_H_
 #define PYBIND11_PROTOBUF_CHECK_UNKNOWN_FIELDS_H_
 
-#include <optional>
 #include <string>
 
 #include "absl/strings/string_view.h"
+#include "absl/types/optional.h"
 #include "google/protobuf/message.h"
 #include "python/google/protobuf/proto_api.h"
 
@@ -46,7 +46,7 @@ class ExtensionsWithUnknownFieldsPolicy {
 void AllowUnknownFieldsFor(absl::string_view top_message_descriptor_full_name,
                            absl::string_view unknown_field_parent_message_fqn);
 
-std::optional<std::string> CheckRecursively(
+absl::optional<std::string> CheckRecursively(
     const ::google::protobuf::python::PyProto_API* py_proto_api,
     const ::google::protobuf::Message* top_message);

@Mizux
Copy link
Collaborator

Mizux commented Dec 13, 2024

AFAIK Protobuf should follow the Foundational CXX Support Policies so yes we should try to be able to target C++14 BUT on Dec 15th 2024 we may drop support to C++14 (10 years) to move to C++17 as minimum required outhere (also internally Google is using C++20 so it not always practical for all Teams to follow this compat effort)

ref: https://github.com/google/oss-policies-info/blob/main/foundational-cxx-support-matrix.md
ref: https://opensource.google/documentation/policies/cplusplus-support#c_language_standard

To move forward:

  1. Check if Protobuf v29.1 (last release) still support C++14, as I don't see the point to support it if Protobuf already drop it.
    note: I hope one day to be able to have a release of pybind_protobuf each time a new release of Protobuf is out...

  2. If Yes, then we may accept/do a PR to "fix" the c++14 support until it is dropped in a very near future...

@Mizux Mizux added the bug Something isn't working label Dec 13, 2024
@Mizux Mizux self-assigned this Dec 13, 2024
@translunar
Copy link
Author

Yeah, it does look like Proto v29 still plans to support C++14. It's v30 where that changes: https://protobuf.dev/news/v30/#drop-cpp-14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants