[libcxx-commits] [PATCH] D113597: [libcxx] Change the type of __size to match __width

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone 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);
----------------
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;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113597