Skip to content

Commit

Permalink
fix(common): workaround GCC "ambiguous" overload errors
Browse files Browse the repository at this point in the history
We used to write code like this:

```cc
template <typename U>
static auto constexpr E(U& u) {
  return std::move(*u.mutable_next_page_token());
}
template <typename U>
static auto constexpr E(U& u) {
  return std::move(u.next_page_token);
}
```

The idea is that only one of these overloads would be available because
of SFINAE.  Some versions of GCC complain that this is ambiguous.
  • Loading branch information
coryan committed Jun 2, 2022
1 parent e2802d1 commit 5970fbe
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 6 deletions.
71 changes: 65 additions & 6 deletions google/cloud/internal/pagination_range.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_INTERNAL_PAGINATION_RANGE_H

#include "google/cloud/internal/invoke_result.h"
#include "google/cloud/internal/type_traits.h"
#include "google/cloud/status_or.h"
#include "google/cloud/stream_range.h"
#include "google/cloud/version.h"
Expand Down Expand Up @@ -107,23 +108,81 @@ class PagedStreamReader {
}

private:
template <typename U, typename AlwaysVoid = void>
struct HasMutableNextPageToken : public std::false_type {};
template <typename U>
struct HasMutableNextPageToken<
U,
void_t<decltype(std::move(*std::declval<U>().mutable_next_page_token()))>>
: public std::true_type {};

template <typename U, typename AlwaysVoid = void>
struct HasNextPageToken : public std::false_type {};
template <typename U>
struct HasNextPageToken<
U, void_t<decltype(std::move(std::declval<U>().next_page_token))>>
: public std::true_type {};

/**
* ExtractPageToken() extracts (i.e., "moves") the page token out of the
* given response object. This function is overloaded based on whether the
* response object has a `.mutable_next_page_token()` member function (e.g.,
* a protobuf), or a `.next_page_token` field (e.g., a regular struct such as
* is used in GCS).
* given response object. This function dispatches to the right function
* to extract the value using either a `.mutable_next_page_token()` member
* function (e.g., a protobuf), or a `.next_page_token` field (e.g., a regular
* struct such as is used in GCS).
*
* The code is more complicated than we would wish. It used to be (this
* comment uses C++17 for exposition purposes, we used the C++11 equivalent):
*
* @code
* template <typename U>
* static auto constexpr E(U& u) {
* return std::move(*u.mutable_next_page_token());
* }
* template <typename U>
* static auto constexpr E(U& u) {
* return std::move(u.next_page_token);
* }
* @endcode
*
* For better or worse some versions of GCC called that ambiguous, even though
* only one would work under SFINAE.
*/
static std::string ExtractPageToken(Response& u) {
static_assert(
HasMutableNextPageToken<Response>::value ||
HasNextPageToken<Response>::value,
"The Response type does not meet the requirements for PaginationRange");
// This may seem expensive, but the value is known at compile type, the
// optimizer should know what to do.
if (HasMutableNextPageToken<Response>::value) {
return UsingMutableNextPageToken<Response>(u);
}
return UsingNextPageToken<Response>(u);
}

template <typename U>
static constexpr auto ExtractPageToken(U& u)
static auto constexpr UsingMutableNextPageToken(U& u)
-> decltype(std::move(*u.mutable_next_page_token())) {
return std::move(*u.mutable_next_page_token());
}
// This overload is unused, it is here just to compile when
// HasMutableNextPageToken<Response>::value is `false`
template <typename U, typename... V>
static std::string UsingMutableNextPageToken(V&...) {
return {};
}

template <typename U>
static constexpr auto ExtractPageToken(U& u)
static auto constexpr UsingNextPageToken(U& u)
-> decltype(std::move(u.next_page_token)) {
return std::move(u.next_page_token);
}
// This overload is unused, it is here just to compile when
// HasNextPageToken<Response>::value is `false`
template <typename... V>
static std::string UsingNextPageToken(V&...) {
return {};
}

Request request_;
std::function<StatusOr<Response>(Request const&)> loader_;
Expand Down
1 change: 1 addition & 0 deletions google/cloud/internal/pagination_range_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ struct ProtoResponse {
std::vector<Item> testonly_items;
std::string testonly_page_token;
std::string* mutable_next_page_token() { return &testonly_page_token; }
std::string const& next_page_token() const { return testonly_page_token; }

// Used for setting the token in tests, but it's not used in the real code.
void testonly_set_page_token(std::string s) {
Expand Down

0 comments on commit 5970fbe

Please sign in to comment.