[libcxx-commits] [PATCH] D151046: [libcxx][tests] Introduce 32-bit feature and use it for stringstream gcount test

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

I'll land this on your behalf.



================
Comment at: libcxx/utils/libcxx/test/features.py:385
+
+  Feature(name='32-bit', when=lambda cfg: int(compilerMacros(cfg)['__SIZEOF_SIZE_T__']) == 4),
 ]
----------------
azat wrote:
> philnik wrote:
> > azat wrote:
> > > ldionne wrote:
> > > > azat wrote:
> > > > > azat wrote:
> > > > > > ldionne wrote:
> > > > > > > Mordante wrote:
> > > > > > > > I'm not convinced we should rely on `__SIZEOF_SIZE_T__` being available at least we should test here it's defined. (Alternatively we do a runtime test, for an example see `has-unix-headers`.)
> > > > > > > I would try using a runtime test that checks the result of `sizeof(void*) == 4`.
> > > > > > >I'm not convinced we should rely on __SIZEOF_SIZE_T__ being available at least we should test here it's defined. (Alternatively we do a runtime test, for an example see has-unix-headers.)
> > > > > > 
> > > > > > I guess a simple fallback to `0` will work, rebased.
> > > > > @ldionne Ok, I could use this, but can you elaborate why `int(compilerMacros(cfg).get('__SIZEOF_SIZE_T__', 0)) == 4` is not good, just to understand is there any difference? (since actually that test relies on the `size_t` size, but this feature flag is about 32-bit architecture so I'm fine both ways)
> > > > Using `sizeof(void*)` would work regardless of the compiler though.
> > > >I'm not convinced we should rely on __SIZEOF_SIZE_T__ being available at least we should test here it's defined. (Alternatively we do a runtime test, for an example see has-unix-headers.)
> > > 
> > > @Mordante BTW it is defined in `clang/lib/Frontend/InitPreprocessor.cpp`, so it should always be defined.
> > The tests aren't just for libc++. The test suite is designed to be as portable as possible, so `__SIZEOF_SIZE_T__` might in fact not be defined. e.g. does MSVC define it? They actually use our test suite.
> > The tests aren't just for libc++. The test suite is designed to be as portable as possible, so `__SIZEOF_SIZE_T__` might in fact not be defined. e.g. does MSVC define it?
> 
> They don't - https://godbolt.org/z/d4d5xG1ET
> 
> >They actually use our test suite.
> 
> Got it thanks!
> 
> Anyway, now `sizeof(void*)` is used.
That @philnik said was my reason too. Thanks for fixing it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151046