steakhal 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 {
----------------
balazske wrote:
> 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.
>
>
I can agree with your two last points. I'm fine with either of those two.
My problem with the first option is that Im afraid that the ASTContex wont
recognize the given type alias as a size_t or similar tspes, and checkers such
as the stdlibrary functuons checker frequently does that sort of type checking
AFAIK. So on certain target triples it wouldnt match and either surface as
broken tests or rotten tests down the line.
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
