balazske added inline comments.
================
Comment at: clang/test/Analysis/Inputs/std-c-library-functions.h:1-2
+typedef typeof(sizeof(int)) size_t;
+typedef signed long ssize_t;
+typedef struct {
----------------
steakhal wrote:
> donat.nagy wrote:
> > balazske wrote:
> > > steakhal wrote:
> > > > balazske wrote:
> > > > > steakhal wrote:
> > > > > > `ssize_t`'s size should match the size of `size_t`. In this
> > > > > > implementation, it would be true only if `size_t` is `long`.
> > > > > >
> > > > > I could not find a working way of defining the type in that way
> > > > > (there is no `__sizte_t`). The current definition should work well in
> > > > > the test code, the property of being the same size is supposedly not
> > > > > used in the tests. The previous definition was not better than this
> > > > > (and different in different places).
> > > > I think [[
> > > > https://github.com/llvm/llvm-project/blob/main/clang/test/Sema/format-strings-scanf.c#L7-L15
> > > > | clang/test/Sema/format-strings-scanf.c ]] uses something like this:
> > > > ```lang=C++
> > > > typedef __SIZE_TYPE__ size_t;
> > > > #define __SSIZE_TYPE__
> > > > \
> > > > __typeof__(_Generic((__SIZE_TYPE__)0,
> > > > \
> > > > unsigned long long int : (long long int)0,
> > > > \
> > > > unsigned long int : (long int)0,
> > > > \
> > > > unsigned int : (int)0,
> > > > \
> > > > unsigned short : (short)0,
> > > > \
> > > > unsigned char : (signed char)0))
> > > > typedef __SSIZE_TYPE__ ssize_t;
> > > > ```
> > > This may work (probably not on all platforms) but still I think in this
> > > context it is only important that we have a type called `ssize_t` and it
> > > is signed, it is less important that it is exactly the correct type.
> > > Other types in the same header are not exact, and `ssize_t` in other test
> > > code (in Analysis tests) is defined not in this way.
> > I agree that we don't need that `_Generic` magic in this particular test
> > file. If we want consistency between the sizes of `size_t` and `ssize_t`
> > then you may define them as e.g. just `unsigned long long` and `long long`
> > -- but I think even that consideration is overkill.
> Whatever. My problem is that this is a header. It should be included from
> individual test files. And test files are the place where the author decides
> if they want to pin the test to a specific target to make assumptions about
> sizes of types or signedness for example. So my concern is that the current
> form of theader is not portable, hence it should be includes from tests where
> the target is pinned to x86 linux. However, we dontenforce this in any way
> ormake it portable.
> Having an ifdef check and error out if the target is not what we expect would
> be suboptimal as premerge bots might only run on only x86 linux. (On second
> thought there are probably windows bots so with caee it might be not as a bit
> issue). I hope I clarified my concerns.
The test did work with the old definition and this patch is only about
restructuring the code. This patch is meant to be a "test NFC". But if this
code gets no acceptance I have these possibilities:
- Define `size_t` as `unsigned long` and `ssize_t` as `signed long`. Probably
mention in a comment that the definitions are not always realistic.
- Use the definition with `_Generic`. But then the other definitions in the
POSIX header should be fixed too, for example `off_t` and `off64_t` are now the
same type, this looks not exact. Will this work on all buildbots with probably
different C language standard?
- Run the tests that use this header only on x86 linux.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D149158/new/
https://reviews.llvm.org/D149158
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
