[libcxx] Change the type of __size to match __width

[androm3da]

Summary

In D103433, size was declared as unsigned which will not find a std::min() overload corresponding with width on some targets.

[• Quuxplusone]

libcxx/include/__format/formatter_integral.h

407

:shipit: but (pre-existing, no action needed on this PR)—

• all of these data members should be underscore-suffixed, e.g. __alignment_, __width_, __fill_, to distinguish them from local variables e.g. __begin, __first, __size.
• perhaps __size should simply be declared as decltype(__width_) __size

[ldionne]

libcxx/include/__format/formatter_integral.h

407

Is there no truncation risk? We normally don't use uint32_t (nor unsigned) for storing the subtraction of pointers. Why is it OK here?

[• Quuxplusone]

libcxx/include/__format/formatter_integral.h

407

IIUC, the uint32 here comes from __format/parser_std_format_spec.h:

class _LIBCPP_TYPE_VIS __parser_width {
public:
  /** Contains a width or an arg-id. */
  uint32_t __width : 31 {0};
  /** Determines whether the value stored is a width or an arg-id. */
  uint32_t __width_as_arg : 1 {0};

and __first - __begin is going to be the length of a formatted unsigned integer (so, at most like 128, not 4 billion). But yeah, maybe it would be safer and cleaner to do simply

auto __size = __first - __begin;
if (this->__width <= __size) {
    this->__width = 0;
} else {
    this->__width -= __size;
}

[androm3da]

libcxx/include/__format/formatter_integral.h

407

Oh, oops. I failed to take in the context here. Should I change __width and __size to ptrdiff_t?

[androm3da]

Is the gdb_pretty_printer_test leveraging __format_unsigned_integral()?

Unfortunately it shows up as UNSUPPORTED when I test. I only tried x86_64-unknown-linux-gnu. Looks like I'll need to satisfy host-has-gdb-with-python to reproduce this?

FAIL: /home/libcxx-builder/.buildkite-agent/builds/253dac57e677-1/llvm-project/libcxx-ci/libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp:247
GDB printed:
   'std::bitset<258u>'
Value should match:
   'std::bitset<258(ul)?>'
FAIL: /home/libcxx-builder/.buildkite-agent/builds/253dac57e677-1/llvm-project/libcxx-ci/libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp:250
GDB printed:
   'std::bitset<0u>'
Value should match:
   'std::bitset<0(ul)?>'
FAIL: /home/libcxx-builder/.buildkite-agent/builds/253dac57e677-1/llvm-project/libcxx-ci/libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp:253
GDB printed:
   'std::bitset<15u> = {[2] = 1, [3] = 1, [4] = 1, [5] = 1, [6] = 1, [7] = 1, [8] = 1, [9] = 1}'
Value should match:
   'std::bitset<15(ul)?> = {\\[2\\] = 1, \\[3\\] = 1, \\[4\\] = 1, \\[5\\] = 1, \\[6\\] = 1, \\[7\\] = 1, \\[8\\] = 1, \\[9\\] = 1}'
FAIL: /home/libcxx-builder/.buildkite-agent/builds/253dac57e677-1/llvm-project/libcxx-ci/libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp:261
GDB printed:
   'std::bitset<258u> = {[0] = 1, [129] = 1, [132] = 1}'
Value should match:
   'std::bitset<258(ul)?> = {\\[0\\] = 1, \\[129\\] = 1, \\[132\\] = 1}'

[Mordante]

libcxx/include/__format/formatter_integral.h

407

:shipit: but (pre-existing, no action needed on this PR)—

• all of these data members should be underscore-suffixed, e.g. __alignment_, __width_, __fill_, to distinguish them from local variables e.g. __begin, __first, __size.

AFAIK we only use a trailing underscore for private members.

407

:shipit: but (pre-existing, no action needed on this PR)—

• all of these data members should be underscore-suffixed, e.g. __alignment_, __width_, __fill_, to distinguish them from local variables e.g. __begin, __first, __size.
• perhaps __size should simply be declared as decltype(__width_) __size

407

Is there no truncation risk? We normally don't use uint32_t (nor unsigned) for storing the subtraction of pointers. Why is it OK here?

assert(__first - __begin >= 0); will always be true.The value is really small, it contains the size of [sign][prefix]. This part is used for zero padding of a value. It will write:

• optionally a sign
• optionally the base prefix, 0x, 0b, 0
• zero or more the zero padding code units
• the actual value

I used an unsigned so I could use min. As Arthur deduced it matches the type of __parser_width. Except that at some point I changed it from unsigned to uint32_t at one place and not the other.

407

I prefer to only change unsigned to uint32_t and use _VSTD::min again. I find the version with the if a less readable.

[Mordante]

LGTM!

[ldionne]

libcxx/include/__format/formatter_integral.h

407

and __first - __begin is going to be the length of a formatted unsigned integer

🚢 it!

[androm3da]

🚢 it!

Sorry for the confusion, this landed as https://reviews.llvm.org/rG2d0aede515e8d7ce141da9708c7e7129d6e24241

That commit didn't include this review reference, sorry. @ldionne did send me a note to include those in the future.

[ldionne]

In D113597#3140607, @androm3da wrote:

🚢 it!

Sorry for the confusion, this landed as https://reviews.llvm.org/rG2d0aede515e8d7ce141da9708c7e7129d6e24241

That commit didn't include this review reference, sorry. @ldionne did send me a note to include those in the future.

Oh, ahah, and then I forgot about it and posted here :-)

No worries! Thanks!

libcxx/include/__format/formatter_integral.h

else {
       // __buf contains [sign][prefix]data
       //                              ^ location of __first
       // The zero padding is done like:
       // - Write [sign][prefix]
       // - Write data right aligned with '0' as fill character.
       __out_it = _VSTD::copy(__begin, __first, _VSTD::move(__out_it));
       this->__alignment = _Flags::_Alignment::__right;
       this->__fill = _CharT('0');
-      unsigned __size = __first - __begin;
+      uint32_t __size = __first - __begin;
       this->__width -= _VSTD::min(__size, this->__width);
     }
 
     return __write(__first, __last, _VSTD::move(__out_it));
   }
 
 #ifndef _LIBCPP_HAS_NO_LOCALIZATION
   /** Format's the locale-specific form's groupings. */