Skip to content

A
abseil-cpp
Commit a3d9a943 by Greg Falcon Committed by Copybara-Service
Options

Fix behaviors of StrCat() and StrFormat() regarding char types and enum types. 

This is a conservative change, in that it only contains bugfixes and allows new patterns that used to be compile errors.  There are other changes we would like to make (as reflected in the comments in char_formatting_test.cc), but we are being careful about the implications of such behavior changes.

The two implementation changes are:
* Apply integral promotions to enums before printing them, so they are always treated as integers (and not chars) by StrCat and StrFormat.
* Classify `unsigned char` and `signed char` as integer-like rather than char-like, so that `StrFormat("%v", v)` accepts those types as integers (consistent with `StrCat()`.)

The behavior changes are:
* StrCat() now accepts char-backed enums (rather than failing to compile).
* StrFormat("%v") now accepts `signed char` and `unsigned char` as integral (rather than failing to compile).
* StrFormat("%v") now correctly formats 8-bit enums as integers (rather than failing internally and emitting nothing).

Tested:
  Modified the char_formatting_test.cc case to reflect changes.
  Re-ran all other tests.
PiperOrigin-RevId: 539659674
Change-Id: Ief56335f5a57e4582af396d00eaa9e7b6be4ddc6
parent 4500c2fa
Showing with 42 additions and 51 deletions
absl/strings/char_formatting_test.cc
...@@ -37,17 +37,12 @@ TEST(CharFormatting, CharEnum) { ...@@ -37,17 +37,12 @@ TEST(CharFormatting, CharEnum) {
auto v = static_cast<CharEnum>('A'); auto v = static_cast<CharEnum>('A');
// Desired behavior: format as decimal // Desired behavior: format as decimal
// (No APIs do this today.) EXPECT_EQ(absl::StrFormat("%vB", v), "65B");
EXPECT_EQ(absl::StrCat(v, "B"), "65B");
// BUG: internally fails
EXPECT_EQ(absl::StrFormat("%vB", v), "");
// Legacy behavior: does not compile:
// EXPECT_EQ(absl::StrCat(ch, "B"), "AB");
// Legacy behavior: format as character: // Legacy behavior: format as character:
// Some older versions of gcc behave differently in this one case. // Some older versions of gcc behave differently in this one case
#if !defined(__GNUC__) || defined(__clang__) #if !defined(__GNUC__) || defined(__clang__)
EXPECT_EQ(absl::Substitute("$0B", v), "AB"); EXPECT_EQ(absl::Substitute("$0B", v), "AB");
#endif #endif
...@@ -57,14 +52,9 @@ enum class CharEnumClass: char {}; ...@@ -57,14 +52,9 @@ enum class CharEnumClass: char {};
TEST(CharFormatting, CharEnumClass) { TEST(CharFormatting, CharEnumClass) {
auto v = static_cast<CharEnumClass>('A'); auto v = static_cast<CharEnumClass>('A');
// Desired behavior: format as decimal // Desired behavior: format as decimal:
// (No APIs do this today.) EXPECT_EQ(absl::StrFormat("%vB", v), "65B");
EXPECT_EQ(absl::StrCat(v, "B"), "65B");
// BUG: internally fails
EXPECT_EQ(absl::StrFormat("%vB", v), "");
// Legacy behavior: does not compile:
// EXPECT_EQ(absl::StrCat(ch, "B"), "AB");
// Legacy behavior: format as character: // Legacy behavior: format as character:
EXPECT_EQ(absl::Substitute("$0B", v), "AB"); EXPECT_EQ(absl::Substitute("$0B", v), "AB");
...@@ -76,9 +66,7 @@ TEST(CharFormatting, UnsignedChar) { ...@@ -76,9 +66,7 @@ TEST(CharFormatting, UnsignedChar) {
// Desired behavior: format as decimal: // Desired behavior: format as decimal:
EXPECT_EQ(absl::StrCat(v, "B"), "65B"); EXPECT_EQ(absl::StrCat(v, "B"), "65B");
EXPECT_EQ(absl::Substitute("$0B", v), "65B"); EXPECT_EQ(absl::Substitute("$0B", v), "65B");
EXPECT_EQ(absl::StrFormat("%vB", v), "65B");
// Legacy behavior: does not compile:
// EXPECT_EQ(absl::StrFormat("%vB", v), "65B");
// Signedness check // Signedness check
const unsigned char w = 255; const unsigned char w = 255;
...@@ -93,9 +81,7 @@ TEST(CharFormatting, SignedChar) { ...@@ -93,9 +81,7 @@ TEST(CharFormatting, SignedChar) {
// Desired behavior: format as decimal: // Desired behavior: format as decimal:
EXPECT_EQ(absl::StrCat(v, "B"), "65B"); EXPECT_EQ(absl::StrCat(v, "B"), "65B");
EXPECT_EQ(absl::Substitute("$0B", v), "65B"); EXPECT_EQ(absl::Substitute("$0B", v), "65B");
EXPECT_EQ(absl::StrFormat("%vB", v), "65B");
// Legacy behavior: does not compile:
// EXPECT_EQ(absl::StrFormat("%vB", v), "AB");
// Signedness check // Signedness check
const signed char w = -128; const signed char w = -128;
...@@ -110,14 +96,13 @@ TEST(CharFormatting, UnsignedCharEnum) { ...@@ -110,14 +96,13 @@ TEST(CharFormatting, UnsignedCharEnum) {
// Desired behavior: format as decimal: // Desired behavior: format as decimal:
EXPECT_EQ(absl::StrCat(v, "B"), "65B"); EXPECT_EQ(absl::StrCat(v, "B"), "65B");
EXPECT_EQ(absl::Substitute("$0B", v), "65B"); EXPECT_EQ(absl::Substitute("$0B", v), "65B");
EXPECT_EQ(absl::StrFormat("%vB", v), "65B");
// BUG: internally fails
EXPECT_EQ(absl::StrFormat("%vB", v), "");
// Signedness check // Signedness check
auto w = static_cast<UnsignedCharEnum>(255); auto w = static_cast<UnsignedCharEnum>(255);
EXPECT_EQ(absl::StrCat(w, "B"), "255B"); EXPECT_EQ(absl::StrCat(w, "B"), "255B");
EXPECT_EQ(absl::Substitute("$0B", w), "255B"); EXPECT_EQ(absl::Substitute("$0B", w), "255B");
EXPECT_EQ(absl::StrFormat("%vB", w), "255B");
} }
enum SignedCharEnum : signed char {}; enum SignedCharEnum : signed char {};
...@@ -127,14 +112,13 @@ TEST(CharFormatting, SignedCharEnum) { ...@@ -127,14 +112,13 @@ TEST(CharFormatting, SignedCharEnum) {
// Desired behavior: format as decimal: // Desired behavior: format as decimal:
EXPECT_EQ(absl::StrCat(v, "B"), "65B"); EXPECT_EQ(absl::StrCat(v, "B"), "65B");
EXPECT_EQ(absl::Substitute("$0B", v), "65B"); EXPECT_EQ(absl::Substitute("$0B", v), "65B");
EXPECT_EQ(absl::StrFormat("%vB", v), "65B");
// BUG: internally fails
EXPECT_EQ(absl::StrFormat("%vB", v), "");
// Signedness check // Signedness check
auto w = static_cast<SignedCharEnum>(-128); auto w = static_cast<SignedCharEnum>(-128);
EXPECT_EQ(absl::StrCat(w, "B"), "-128B"); EXPECT_EQ(absl::StrCat(w, "B"), "-128B");
EXPECT_EQ(absl::Substitute("$0B", w), "-128B"); EXPECT_EQ(absl::Substitute("$0B", w), "-128B");
EXPECT_EQ(absl::StrFormat("%vB", w), "-128B");
} }
enum class UnsignedCharEnumClass : unsigned char {}; enum class UnsignedCharEnumClass : unsigned char {};
...@@ -144,14 +128,13 @@ TEST(CharFormatting, UnsignedCharEnumClass) { ...@@ -144,14 +128,13 @@ TEST(CharFormatting, UnsignedCharEnumClass) {
// Desired behavior: format as decimal: // Desired behavior: format as decimal:
EXPECT_EQ(absl::StrCat(v, "B"), "65B"); EXPECT_EQ(absl::StrCat(v, "B"), "65B");
EXPECT_EQ(absl::Substitute("$0B", v), "65B"); EXPECT_EQ(absl::Substitute("$0B", v), "65B");
EXPECT_EQ(absl::StrFormat("%vB", v), "65B");
// BUG: internally fails
EXPECT_EQ(absl::StrFormat("%vB", v), "");
// Signedness check // Signedness check
auto w = static_cast<UnsignedCharEnumClass>(255); auto w = static_cast<UnsignedCharEnumClass>(255);
EXPECT_EQ(absl::StrCat(w, "B"), "255B"); EXPECT_EQ(absl::StrCat(w, "B"), "255B");
EXPECT_EQ(absl::Substitute("$0B", w), "255B"); EXPECT_EQ(absl::Substitute("$0B", w), "255B");
EXPECT_EQ(absl::StrFormat("%vB", w), "255B");
} }
enum SignedCharEnumClass : signed char {}; enum SignedCharEnumClass : signed char {};
...@@ -161,14 +144,13 @@ TEST(CharFormatting, SignedCharEnumClass) { ...@@ -161,14 +144,13 @@ TEST(CharFormatting, SignedCharEnumClass) {
// Desired behavior: format as decimal: // Desired behavior: format as decimal:
EXPECT_EQ(absl::StrCat(v, "B"), "65B"); EXPECT_EQ(absl::StrCat(v, "B"), "65B");
EXPECT_EQ(absl::Substitute("$0B", v), "65B"); EXPECT_EQ(absl::Substitute("$0B", v), "65B");
EXPECT_EQ(absl::StrFormat("%vB", v), "65B");
// BUG: internally fails
EXPECT_EQ(absl::StrFormat("%vB", v), "");
// Signedness check // Signedness check
auto w = static_cast<SignedCharEnumClass>(-128); auto w = static_cast<SignedCharEnumClass>(-128);
EXPECT_EQ(absl::StrCat(w, "B"), "-128B"); EXPECT_EQ(absl::StrCat(w, "B"), "-128B");
EXPECT_EQ(absl::Substitute("$0B", w), "-128B"); EXPECT_EQ(absl::Substitute("$0B", w), "-128B");
EXPECT_EQ(absl::StrFormat("%vB", w), "-128B");
} }
#ifdef __cpp_lib_byte #ifdef __cpp_lib_byte
...@@ -177,12 +159,10 @@ TEST(CharFormatting, StdByte) { ...@@ -177,12 +159,10 @@ TEST(CharFormatting, StdByte) {
// Desired behavior: format as 0xff // Desired behavior: format as 0xff
// (No APIs do this today.) // (No APIs do this today.)
// BUG: internally fails
EXPECT_EQ(absl::StrFormat("%vB", v), "");
// Legacy behavior: format as decimal: // Legacy behavior: format as decimal:
EXPECT_EQ(absl::StrCat(v, "B"), "65B"); EXPECT_EQ(absl::StrCat(v, "B"), "65B");
EXPECT_EQ(absl::Substitute("$0B", v), "65B"); EXPECT_EQ(absl::Substitute("$0B", v), "65B");
EXPECT_EQ(absl::StrFormat("%vB", v), "65B");
} }
#endif // _cpp_lib_byte #endif // _cpp_lib_byte
......
absl/strings/internal/str_format/arg.cc
...@@ -461,18 +461,18 @@ CharConvertResult FormatConvertImpl(char v, const FormatConversionSpecImpl conv, ...@@ -461,18 +461,18 @@ CharConvertResult FormatConvertImpl(char v, const FormatConversionSpecImpl conv,
FormatSinkImpl *sink) { FormatSinkImpl *sink) {
return {ConvertIntArg(v, conv, sink)}; return {ConvertIntArg(v, conv, sink)};
} }
CharConvertResult FormatConvertImpl(signed char v,
// ==================== Ints ====================
IntegralConvertResult FormatConvertImpl(signed char v,
const FormatConversionSpecImpl conv, const FormatConversionSpecImpl conv,
FormatSinkImpl *sink) { FormatSinkImpl *sink) {
return {ConvertIntArg(v, conv, sink)}; return {ConvertIntArg(v, conv, sink)};
} }
CharConvertResult FormatConvertImpl(unsigned char v, IntegralConvertResult FormatConvertImpl(unsigned char v,
const FormatConversionSpecImpl conv, const FormatConversionSpecImpl conv,
FormatSinkImpl *sink) { FormatSinkImpl *sink) {
return {ConvertIntArg(v, conv, sink)}; return {ConvertIntArg(v, conv, sink)};
} }
// ==================== Ints ====================
IntegralConvertResult FormatConvertImpl(short v, // NOLINT IntegralConvertResult FormatConvertImpl(short v, // NOLINT
const FormatConversionSpecImpl conv, const FormatConversionSpecImpl conv,
FormatSinkImpl *sink) { FormatSinkImpl *sink) {
......
absl/strings/internal/str_format/arg.h
...@@ -279,14 +279,14 @@ FloatingConvertResult FormatConvertImpl(long double v, ...@@ -279,14 +279,14 @@ FloatingConvertResult FormatConvertImpl(long double v,
// Chars. // Chars.
CharConvertResult FormatConvertImpl(char v, FormatConversionSpecImpl conv, CharConvertResult FormatConvertImpl(char v, FormatConversionSpecImpl conv,
FormatSinkImpl* sink); FormatSinkImpl* sink);
CharConvertResult FormatConvertImpl(signed char v,
// Ints.
IntegralConvertResult FormatConvertImpl(signed char v,
FormatConversionSpecImpl conv, FormatConversionSpecImpl conv,
FormatSinkImpl* sink); FormatSinkImpl* sink);
CharConvertResult FormatConvertImpl(unsigned char v, IntegralConvertResult FormatConvertImpl(unsigned char v,
FormatConversionSpecImpl conv, FormatConversionSpecImpl conv,
FormatSinkImpl* sink); FormatSinkImpl* sink);
// Ints.
IntegralConvertResult FormatConvertImpl(short v, // NOLINT IntegralConvertResult FormatConvertImpl(short v, // NOLINT
FormatConversionSpecImpl conv, FormatConversionSpecImpl conv,
FormatSinkImpl* sink); FormatSinkImpl* sink);
...@@ -441,7 +441,7 @@ class FormatArgImpl { ...@@ -441,7 +441,7 @@ class FormatArgImpl {
// For everything else: // For everything else:
// - Decay char* and char arrays into `const char*` // - Decay char* and char arrays into `const char*`
// - Decay any other pointer to `const void*` // - Decay any other pointer to `const void*`
// - Decay all enums to their underlying type. // - Decay all enums to the integral promotion of their underlying type.
// - Decay function pointers to void*. // - Decay function pointers to void*.
template <typename T, typename = void> template <typename T, typename = void>
struct DecayType { struct DecayType {
...@@ -461,7 +461,7 @@ class FormatArgImpl { ...@@ -461,7 +461,7 @@ class FormatArgImpl {
!str_format_internal::HasUserDefinedConvert<T>::value && !str_format_internal::HasUserDefinedConvert<T>::value &&
!strings_internal::HasAbslStringify<T>::value && !strings_internal::HasAbslStringify<T>::value &&
std::is_enum<T>::value>::type> { std::is_enum<T>::value>::type> {
using type = typename std::underlying_type<T>::type; using type = decltype(+typename std::underlying_type<T>::type());
}; };
public: public:
......
absl/strings/str_cat.h
...@@ -374,14 +374,24 @@ class AlphaNum { ...@@ -374,14 +374,24 @@ class AlphaNum {
const char* data() const { return piece_.data(); } const char* data() const { return piece_.data(); }
absl::string_view Piece() const { return piece_; } absl::string_view Piece() const { return piece_; }
// Normal enums are already handled by the integer formatters. // Match unscoped enums. Use integral promotion so that a `char`-backed
// This overload matches only scoped enums. // enum becomes a wider integral type AlphaNum will accept.
template <typename T, template <typename T,
typename = typename std::enable_if< typename = typename std::enable_if<
std::is_enum<T>{} && !std::is_convertible<T, int>{} && std::is_enum<T>{} && std::is_convertible<T, int>{} &&
!strings_internal::HasAbslStringify<T>::value>::type> !strings_internal::HasAbslStringify<T>::value>::type>
AlphaNum(T e) // NOLINT(runtime/explicit) AlphaNum(T e) // NOLINT(runtime/explicit)
: AlphaNum(static_cast<typename std::underlying_type<T>::type>(e)) {} : AlphaNum(+e) {}
// This overload matches scoped enums. We must explicitly cast to the
// underlying type, but use integral promotion for the same reason as above.
template <typename T,
typename std::enable_if<
std::is_enum<T>{} && !std::is_convertible<T, int>{} &&
!strings_internal::HasAbslStringify<T>::value,
char*>::type = nullptr>
AlphaNum(T e) // NOLINT(runtime/explicit)
: AlphaNum(+static_cast<typename std::underlying_type<T>::type>(e)) {}
// vector<bool>::reference and const_reference require special help to // vector<bool>::reference and const_reference require special help to
// convert to `AlphaNum` because it requires two user defined conversions. // convert to `AlphaNum` because it requires two user defined conversions.
......
absl/strings/str_format.h
...@@ -259,6 +259,7 @@ class FormatCountCapture { ...@@ -259,6 +259,7 @@ class FormatCountCapture {
// * Characters: `char`, `signed char`, `unsigned char` // * Characters: `char`, `signed char`, `unsigned char`
// * Integers: `int`, `short`, `unsigned short`, `unsigned`, `long`, // * Integers: `int`, `short`, `unsigned short`, `unsigned`, `long`,
// `unsigned long`, `long long`, `unsigned long long` // `unsigned long`, `long long`, `unsigned long long`
// * Enums: printed as their underlying integral value
// * Floating-point: `float`, `double`, `long double` // * Floating-point: `float`, `double`, `long double`
// //
// However, in the `str_format` library, a format conversion specifies a broader // However, in the `str_format` library, a format conversion specifies a broader
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment