pybind11_protobuf icon indicating copy to clipboard operation
pybind11_protobuf copied to clipboard

Request: change std::optional to absl::optional

Open translunar opened this issue 1 year ago • 3 comments

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 avatar Dec 11 '24 02:12 translunar

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);

translunar avatar Dec 11 '24 02:12 translunar

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 avatar Dec 13 '24 09:12 Mizux

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

translunar avatar Dec 13 '24 19:12 translunar