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

remove_const from value_type of Range::value_type #2255

Closed
wants to merge 1 commit into from

Conversation

tchaikov
Copy link
Contributor

in general, value_type in a range-alike type should not be qualified with const specifier. the reference type does.

before this change, when formatting StringPiece, fmt 11 does not consider it as a formattable type, as it falls into the string-alike category, but its value_type is const char which is different from the Char type of the context, which is char when formatting using fmt::format("{}", ipSlashCidr).

hence we have the build failure like:

/usr/include/fmt/base.h: In instantiation of ‘constexpr fmt::v11::detail::value<Context> fmt::v11::detail::make_arg(T&) [with bool PACKED = true; Context = fmt::v11::context; T = folly::Range<const char*>; typename std::enable_if<PACKED, int>::type <anonymous> = 0]’:
/usr/include/fmt/base.h:2002:74:   required from ‘constexpr fmt::v11::detail::format_arg_store<Context, NUM_ARGS, 0, DESC> fmt::v11::make_format_args(T& ...) [with Context = context; T = {folly::Range<const char*>}; long unsigned int NUM_ARGS = 1; long unsigned int NUM_NAMED_ARGS = 0; long long unsigned int DESC = 15; typename std::enable_if<(NUM_NAMED_ARGS == 0), int>::type <anonymous> = 0]’
 2002 |   return {{detail::make_arg<NUM_ARGS <= detail::max_packed_args, Context>(
      |            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
 2003 |       args)...}};
      |       ~~~~~
/usr/include/fmt/format.h:4357:44:   required from ‘std::string fmt::v11::format(format_string<T ...>, T&& ...) [with T = {folly::Range<const char*>&}; std::string = std::__cxx11::basic_string<char>; format_string<T ...> = basic_format_string<char, folly::Range<const char*>&>]’
 4357 |   return vformat(fmt, fmt::make_format_args(args...));
      |                       ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~
/home/kefu/folly/folly/IPAddress.cpp:100:47:   required from here
  100 |     throw IPAddressFormatException(fmt::format(
      |                                    ~~~~~~~~~~~^
  101 |         "Invalid ipSlashCidr specified. Expected IP/CIDR format, got '{}'",
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  102 |         ipSlashCidr));
      |         ~~~~~~~~~~~~
/usr/include/fmt/base.h:1611:17: error: static assertion failed: Mixing character types is disallowed.
 1611 |   static_assert(formattable_char, "Mixing character types is disallowed.");
      |                 ^~~~~~~~~~~~~~~~
/usr/include/fmt/base.h:1611:17: note: ‘formattable_char’ evaluates to false
/usr/include/fmt/base.h: In instantiation of ‘constexpr fmt::v11::detail::value<Context> fmt::v11::detail::make_arg(T&) [with bool PACKED = true; Context = fmt::v11::context; T = const folly::Range<const char*>; typename std::enable_if<PACKED, int>::type <anonymous> = 0]’:
/usr/include/fmt/base.h:2002:74:   required from ‘constexpr fmt::v11::detail::format_arg_store<Context, NUM_ARGS, 0, DESC> fmt::v11::make_format_args(T& ...) [with Context = context; T = {const folly::Range<const char*>}; long unsigned int NUM_ARGS = 1; long unsigned int NUM_NAMED_ARGS = 0; long long unsigned int DESC = 15; typename std::enable_if<(NUM_NAMED_ARGS == 0), int>::type <anonymous> = 0]’
 2002 |   return {{detail::make_arg<NUM_ARGS <= detail::max_packed_args, Context>(
      |            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
 2003 |       args)...}};
      |       ~~~~~
/usr/include/fmt/format.h:4357:44:   required from ‘std::string fmt::v11::format(format_string<T ...>, T&& ...) [with T = {const folly::Range<const char*>&}; std::string = std::__cxx11::basic_string<char>; format_string<T ...> = basic_format_string<char, const folly::Range<const char*>&>]’
 4357 |   return vformat(fmt, fmt::make_format_args(args...));
      |                       ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~
/home/kefu/folly/folly/IPAddress.cpp:113:22:   required from here
  113 |           fmt::format("Invalid IP address {}", vec.at(0)));
      |           ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/fmt/base.h:1611:17: error: static assertion failed: Mixing character types is disallowed.
 1611 |   static_assert(formattable_char, "Mixing character types is disallowed.");
      |                 ^~~~~~~~~~~~~~~~
/usr/include/fmt/base.h:1611:17: note: ‘formattable_char’ evaluates to false

in this change, we use the non-const value type, and update the some methods where the reference type is supposed to be used.

Fixes #2172
Signed-off-by: Kefu Chai [email protected]

in general, value_type in a range-alike type should not be qualified
with const specifier. the `reference` type does.

before this change, when formatting `StringPiece`, fmt 11 does not
consider it as a formattable type, as it falls into the string-alike
category, but its value_type is `const char` which is different from
the `Char` type of the context, which is `char` when formatting using
`fmt::format("{}", ipSlashCidr)`.

hence we have the build failure like:

```
/usr/include/fmt/base.h: In instantiation of ‘constexpr fmt::v11::detail::value<Context> fmt::v11::detail::make_arg(T&) [with bool PACKED = true; Context = fmt::v11::context; T = folly::Range<const char*>; typename std::enable_if<PACKED, int>::type <anonymous> = 0]’:
/usr/include/fmt/base.h:2002:74:   required from ‘constexpr fmt::v11::detail::format_arg_store<Context, NUM_ARGS, 0, DESC> fmt::v11::make_format_args(T& ...) [with Context = context; T = {folly::Range<const char*>}; long unsigned int NUM_ARGS = 1; long unsigned int NUM_NAMED_ARGS = 0; long long unsigned int DESC = 15; typename std::enable_if<(NUM_NAMED_ARGS == 0), int>::type <anonymous> = 0]’
 2002 |   return {{detail::make_arg<NUM_ARGS <= detail::max_packed_args, Context>(
      |            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
 2003 |       args)...}};
      |       ~~~~~
/usr/include/fmt/format.h:4357:44:   required from ‘std::string fmt::v11::format(format_string<T ...>, T&& ...) [with T = {folly::Range<const char*>&}; std::string = std::__cxx11::basic_string<char>; format_string<T ...> = basic_format_string<char, folly::Range<const char*>&>]’
 4357 |   return vformat(fmt, fmt::make_format_args(args...));
      |                       ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~
/home/kefu/folly/folly/IPAddress.cpp:100:47:   required from here
  100 |     throw IPAddressFormatException(fmt::format(
      |                                    ~~~~~~~~~~~^
  101 |         "Invalid ipSlashCidr specified. Expected IP/CIDR format, got '{}'",
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  102 |         ipSlashCidr));
      |         ~~~~~~~~~~~~
/usr/include/fmt/base.h:1611:17: error: static assertion failed: Mixing character types is disallowed.
 1611 |   static_assert(formattable_char, "Mixing character types is disallowed.");
      |                 ^~~~~~~~~~~~~~~~
/usr/include/fmt/base.h:1611:17: note: ‘formattable_char’ evaluates to false
/usr/include/fmt/base.h: In instantiation of ‘constexpr fmt::v11::detail::value<Context> fmt::v11::detail::make_arg(T&) [with bool PACKED = true; Context = fmt::v11::context; T = const folly::Range<const char*>; typename std::enable_if<PACKED, int>::type <anonymous> = 0]’:
/usr/include/fmt/base.h:2002:74:   required from ‘constexpr fmt::v11::detail::format_arg_store<Context, NUM_ARGS, 0, DESC> fmt::v11::make_format_args(T& ...) [with Context = context; T = {const folly::Range<const char*>}; long unsigned int NUM_ARGS = 1; long unsigned int NUM_NAMED_ARGS = 0; long long unsigned int DESC = 15; typename std::enable_if<(NUM_NAMED_ARGS == 0), int>::type <anonymous> = 0]’
 2002 |   return {{detail::make_arg<NUM_ARGS <= detail::max_packed_args, Context>(
      |            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
 2003 |       args)...}};
      |       ~~~~~
/usr/include/fmt/format.h:4357:44:   required from ‘std::string fmt::v11::format(format_string<T ...>, T&& ...) [with T = {const folly::Range<const char*>&}; std::string = std::__cxx11::basic_string<char>; format_string<T ...> = basic_format_string<char, const folly::Range<const char*>&>]’
 4357 |   return vformat(fmt, fmt::make_format_args(args...));
      |                       ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~
/home/kefu/folly/folly/IPAddress.cpp:113:22:   required from here
  113 |           fmt::format("Invalid IP address {}", vec.at(0)));
      |           ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/fmt/base.h:1611:17: error: static assertion failed: Mixing character types is disallowed.
 1611 |   static_assert(formattable_char, "Mixing character types is disallowed.");
      |                 ^~~~~~~~~~~~~~~~
/usr/include/fmt/base.h:1611:17: note: ‘formattable_char’ evaluates to false
```

in this change, we use the non-const value type, and update the
some methods where the `reference` type is supposed to be used.

Fixes facebook#2172
Signed-off-by: Kefu Chai <[email protected]>
@facebook-github-bot
Copy link
Contributor

@Orvid has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@michel-slm
Copy link
Contributor

Looks like we're going with #2265 instead

@tchaikov
Copy link
Contributor Author

tchaikov commented Jul 24, 2024

@michel-slm thanks, i still think the std::iterator_traits<Iter>::value_type is a better solution. but please feel free to close this one once #2265 lands.

@vitaut
Copy link
Contributor

vitaut commented Jul 30, 2024

Superseded by 21e8dcd but thanks anyway.

@vitaut vitaut closed this Jul 30, 2024
@tchaikov tchaikov deleted the remove_const branch July 31, 2024 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build Error on Debian 12: mixing character types in fmt
4 participants