[PATCH] D146492: Add new printNumber() for size_t

James Henderson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 22 01:06:47 PDT 2023


jhenderson added a comment.

Thanks @junhee-yoo, the difference that Mac uses `unsigned long` not `unsigned long long` was the piece I was missing. I bet if you look at the `uint32_t` type you'll see it is defined as `unsigned int`. This will mean that the current `printNumber` overloads for Mac probably cover the following fundamental integer types:

- `char` (or possibly `signed char`) (from `int8_t`)
- `short` (from `int16_t`)
- `int` (from `int32_t`)
- `long long` (from `int64_t`)

and their unsigned equivalents from the `uint*_t` types.

The problem ultimately boils down to the fact that we've attempted to cover all sizes of integers with `printNumber` overloads, but haven't actually done so, because we've used the fixed-width integer types, rather than the fundamental C++ types. The latter are what are ultimately used in picking which function overload to use. I'd therefore like to propose the following changes:

1. Replace the existing `printNumber` integer type overloads with fundamental type versions (specifically `char`, `signed char`, `unsigned char`, `short`, `unsigned short`, `int`, `unsigned int`, `long`, `unsigned long`, `long long`, and `unsigned long long` (see https://en.cppreference.com/w/cpp/language/types for a good reference).
2. Revert any "fixes" (casts etc) that have been added to make the code work on Mac or other platforms, since they'll be unnecessary at this point and just clutter the code (and none of them were reviewed...).

I'd do this myself, but actually coding on LLVM falls outside my job remit these days. @paulkirth, it probably should be your responsibility as the person who originally introduced the breakage to coordinate this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146492



More information about the llvm-commits mailing list