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

[Brian Cain via Phabricator via libcxx-commits]

androm3da 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);
----------------
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`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113597