Copilot commented on code in PR #49662:
URL: https://github.com/apache/arrow/pull/49662#discussion_r3036611017
##########
cpp/src/arrow/flight/transport/grpc/util_internal.h:
##########
@@ -34,6 +34,12 @@ class Status;
namespace flight {
+#define GRPC_CPP_VERSION_CHECK(major, minor, patch)
\
+ ((GRPC_CPP_VERSION_MAJOR > (major) ||
\
+ (GRPC_CPP_VERSION_MAJOR == (major) && GRPC_CPP_VERSION_MINOR > (minor)) ||
\
+ ((GRPC_CPP_VERSION_MAJOR == (major) && GRPC_CPP_VERSION_MINOR == (minor)
&& \
+ GRPC_CPP_VERSION_PATCH >= (patch)))))
Review Comment:
`GRPC_CPP_VERSION_CHECK` relies on `GRPC_CPP_VERSION_{MAJOR,MINOR,PATCH}`
but this header doesn’t include the header that defines them. Please include
the appropriate gRPC version header (e.g. `grpcpp/version_info.h`) or add a
defensive `#ifdef GRPC_CPP_VERSION_MAJOR` fallback to avoid brittle
transitive-include dependencies.
##########
cpp/src/arrow/flight/transport/grpc/grpc_client.cc:
##########
@@ -732,19 +732,31 @@ class GrpcClientImpl : public internal::ClientTransport {
# endif // defined(GRPC_USE_CERTIFICATE_VERIFIER)
# if defined(GRPC_USE_TLS_CHANNEL_CREDENTIALS_OPTIONS)
+# if GRPC_CPP_VERSION_CHECK(1, 80, 0)
+ auto certificate_provider =
+
std::make_shared<::grpc::experimental::InMemoryCertificateProvider>();
+
RETURN_NOT_OK(FromAbslStatus(certificate_provider->UpdateRoot(kDummyRootCert)));
+# else
Review Comment:
This code calls `FromAbslStatus(...)`, but `FromAbslStatus` is only declared
when `ABSL_NAMESPACE_BEGIN` is defined. Please either (1) ensure Abseil status
headers are included so the symbol is always available in this build
configuration, or (2) guard this call with the same preprocessor condition to
avoid compilation failures in builds where Abseil isn’t present/used.
##########
cpp/src/arrow/flight/transport/grpc/util_internal.h:
##########
@@ -90,6 +96,12 @@ ARROW_FLIGHT_EXPORT
::grpc::Status ToGrpcStatus(const Status& arrow_status,
::grpc::ServerContext* ctx = nullptr);
+// Abseil is included.
+#ifdef ABSL_NAMESPACE_BEGIN
+/// Convert a Abseil status to an Arrow status.
Review Comment:
Grammar: “Convert a Abseil status” should be “Convert an Abseil status”.
```suggestion
/// Convert an Abseil status to an Arrow status.
```
##########
cpp/src/arrow/flight/transport/grpc/util_internal.h:
##########
@@ -90,6 +96,12 @@ ARROW_FLIGHT_EXPORT
::grpc::Status ToGrpcStatus(const Status& arrow_status,
::grpc::ServerContext* ctx = nullptr);
+// Abseil is included.
+#ifdef ABSL_NAMESPACE_BEGIN
+/// Convert a Abseil status to an Arrow status.
+ARROW_FLIGHT_EXPORT
+Status FromAbslStatus(const ::absl::Status& absl_status);
Review Comment:
`FromAbslStatus` uses `::absl::Status`/`::absl::StatusCode` but neither this
header nor the corresponding `.cc` includes the Abseil status header. Relying
on indirect includes is fragile and can break IWYU builds; please explicitly
include `<absl/status/status.h>` (guarded with `__has_include` if needed).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
