alexfh added inline comments.
================ Comment at: clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/system-header-posix-api.h:1 -//===--- signal.h - Stub header for tests -----------------------*- C++ -*-===// +//===--- system-header-posix-api.h - Stub header for tests ------*- C++ -*-===// // ---------------- aaron.ballman wrote: > balazske wrote: > > aaron.ballman wrote: > > > balazske wrote: > > > > aaron.ballman wrote: > > > > > I think we should strive to replicate the system headers rather than > > > > > fake up a system header (these headers can be used by more than one > > > > > check). So I think we want to keep signal.h and stdlib.h, and should > > > > > include the POSIX-specific functionality with a macro. This also > > > > > helps us test the behavior on systems like Windows which are not > > > > > POSIX systems. > > > > My concern was to add these many small header files with just some > > > > functions in them. For `accept` more than one header is needed if we > > > > want to exactly replicate the system files. And data types like > > > > `size_t` should have a common header too. So I decided to have one > > > > header that contains all system functions and data types. This can be > > > > used by multiple tests and extended as needed. > > > I don't think we're too worried about having a bunch of small test > > > headers around -- I think it's more important the headers used to check > > > system header behavior be understandable as to what you're getting from > > > them. For instance, there are clang-tidy checks for llvm-libc that may > > > have very different needs from what you're doing here. > > > > > > What's more, these changes break existing tests -- I don't see any > > > companion changes to fix those up. > > On my system (Ubuntu) I do not see failing tests, and these changes do not > > touch files that were created before D87449, no new problems should happen. > > I am not against "mirroring" the POSIX API header structure into the test > > stub header structure, only do not like the overhead of adding these many > > files with just 1-2 lines (that is needed for this test) in them. It is not > > done this way in the clang tests either (there are multiple > > "system-header-simulator" files that contain every declaration usable for > > specific purposes at one or more tests). Also I want to have the opinion of > > another reviewer for this question. > > On my system (Ubuntu) I do not see failing tests, and these changes do not > > touch files that were created before D87449, no new problems should happen. > > You renamed stdlib.h and `lllvmlibc-restrict-system-libc-headers.cpp` > includes `stdlib.h`, but after closer inspection, I see now that it's > including one from a different directory. So this doesn't break the things I > thought it was breaking, good! > > > Also I want to have the opinion of another reviewer for this question. > > I'm happy to go with whatever @alexfh thinks. > Based on the experience with our internal set of checks and tests for them, I tend to think that keeping mock API definitions arranged in a similar way to the corresponding real library is a more sustainable way to manage mock headers. When arranged this way, it's easier to find the origins of, verify, and update definitions, when, for example, a higher fidelity replication of a system/STL/... entity becomes necessary. Dependencies between these mock headers (if necessary) can repeat those in the real library, and #includes of these mock headers can be composed in a similar way. Though the structure of the mock headers can start be approximate and the level of detail can be added when necessary. For example, one can start with a mock header for <string> that would contain initializer_list implementation. But when another test would need <initializer_list>, the corresponding entities can be moved to a separate initializer_list header. As for different header contents depending on target platform, preprocessor macros seem to be a better way to handle this than using separate mock headers. The set of mock headers can be shared by all tests that examine the corresponding API. I don't see good reasons to keep different mocks of the same API for different tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90851/new/ https://reviews.llvm.org/D90851 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
