[libcxx-commits] [PATCH] D134420: [libc++] Use intptr_t instead of ptrdiff_t for messages_base::catalog

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision as: Mordante.
Mordante added a subscriber: ldionne.
Mordante added a comment.

LGTM, but I think it would be good for @ldionne to have a look too.



================
Comment at: libcxx/include/locale:3464
 public:
-    typedef ptrdiff_t catalog;
+    typedef intptr_t catalog;
 
----------------
arichardson wrote:
> arichardson wrote:
> > Mordante wrote:
> > > Did you check these types are the same on all supported platforms.
> > > I noticed `intptr_t` is not guaranteed to be available on all platforms. Is that an issue?
> > > 
> > I've looked at clang and for almost all targetinfo classes `PtrDiffType` uses the same type as `IntPtrType`. The only exception I can see is MIPS O32 ABI which uses Int for ptrdiff_t but long for intptr_t (those are the same size just different integer types). In case this is a problem, I'm not sure if MIPS O32 ABI is actually supported - if 32-bit is supported I would assume it's only N32 ABI.
> > 
> > FreeBSD stddef.h checks __SIZEOF_PTRDIFF_T__ and __SIZEOF_POINTER__, so if those match inside clang they will use the same types (which is true for all FreeBSD architectures other than CHERI-extended ones, which are not supported upstream)
> > 
> > I think in theory they could be different with other OS/compiler-provided stddef.h, but I'd be surprised if they were since we don't yet support any architectures that don't have a flat address space (which should imply size_t==intptr_t).
> I guess this could change mangled names for 32-bit MIPS, but https://libcxx.llvm.org/#platform-and-compiler-support does not list MIPS anywhere, so maybe this is okay? Otherwise I could add ifdefs (but I'd prefer not to).
Thanks for the info, then it seems it's not an issue to make this change.


================
Comment at: libcxx/include/locale:3539
     nl_catd __cat = (nl_catd)__c;
+    static_assert(sizeof(catalog) >= sizeof(nl_catd), "Unexpected nl_catd type");
     char* __n = catgets(__cat, __set, __msgid, __ndflt.c_str());
----------------
philnik wrote:
> Mordante wrote:
> > Is this code used in C++03? static_assert is a C++11 feature.
> We have `static_assert` in C++03. We `#define static_assert(...) _Static_assert(__VA_ARGS__)` in `__config`.
Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134420