[libcxx-commits] [PATCH] D113597: [libcxx] Update width adjustment for __format_unsigned_integral

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/__format/formatter_integral.h:407
       this->__fill = _CharT('0');
-      unsigned __size = __first - __begin;
+      uint32_t __size = __first - __begin;
       this->__width -= _VSTD::min(__size, this->__width);
----------------
androm3da wrote:
> Quuxplusone wrote:
> > ldionne wrote:
> > > Quuxplusone wrote:
> > > > :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`
> > > 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?
> > 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;
> > }
> > ```
> Oh, oops.  I failed to take in the context here.  Should I change `__width` and `__size` to `ptrdiff_t`?
> :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.





================
Comment at: libcxx/include/__format/formatter_integral.h:407
       this->__fill = _CharT('0');
-      unsigned __size = __first - __begin;
+      uint32_t __size = __first - __begin;
       this->__width -= _VSTD::min(__size, this->__width);
----------------
Mordante wrote:
> androm3da wrote:
> > Quuxplusone wrote:
> > > ldionne wrote:
> > > > Quuxplusone wrote:
> > > > > :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`
> > > > 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?
> > > 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;
> > > }
> > > ```
> > Oh, oops.  I failed to take in the context here.  Should I change `__width` and `__size` to `ptrdiff_t`?
> > :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.
> 
> 
> 
> :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`




================
Comment at: libcxx/include/__format/formatter_integral.h:407
       this->__fill = _CharT('0');
-      unsigned __size = __first - __begin;
+      uint32_t __size = __first - __begin;
       this->__width -= _VSTD::min(__size, this->__width);
----------------
Mordante wrote:
> Mordante wrote:
> > androm3da wrote:
> > > Quuxplusone wrote:
> > > > ldionne wrote:
> > > > > Quuxplusone wrote:
> > > > > > :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`
> > > > > 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?
> > > > 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;
> > > > }
> > > > ```
> > > Oh, oops.  I failed to take in the context here.  Should I change `__width` and `__size` to `ptrdiff_t`?
> > > :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.
> > 
> > 
> > 
> > :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`
> 
> 
> 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.


================
Comment at: libcxx/include/__format/formatter_integral.h:407
       this->__fill = _CharT('0');
-      unsigned __size = __first - __begin;
+      uint32_t __size = __first - __begin;
       this->__width -= _VSTD::min(__size, this->__width);
----------------
Mordante wrote:
> Mordante wrote:
> > Mordante wrote:
> > > androm3da wrote:
> > > > Quuxplusone wrote:
> > > > > ldionne wrote:
> > > > > > Quuxplusone wrote:
> > > > > > > :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`
> > > > > > 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?
> > > > > 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;
> > > > > }
> > > > > ```
> > > > Oh, oops.  I failed to take in the context here.  Should I change `__width` and `__size` to `ptrdiff_t`?
> > > > :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.
> > > 
> > > 
> > > 
> > > :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`
> > 
> > 
> > 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.
I prefer to only change `unsigned` to `uint32_t` and use `_VSTD::min` again. I find the version with the `if` a less readable. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113597/new/

https://reviews.llvm.org/D113597