abrachet added inline comments.
================
Comment at:
clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp:40
+ SrcMgr::CharacteristicKind FileType) {
+ if (SrcMgr::isSystem(FileType)) {
+ if (!SM.isInMainFile(HashLoc)) {
----------------
PaulkaToast wrote:
> aaron.ballman wrote:
> > njames93 wrote:
> > > abrachet wrote:
> > > > Could you whitelist the freestanding/compiler provided headers like
> > > > stddef, stdatomic...
> > > Or have a user configurable whitelist
> > It should be configurable and named something other than whitelist. I'd
> > recommend `AllowedHeaders` or something along those lines.
> Maintaining a list of acceptable and blacklisted headers would produce a fair
> bit of maintenance burden. We have a specific use-case in mind here for
> llvm-libc which is to avoid use of system libc headers that aren't provided
> by the compiler. I've added a check to allow for compiler provided headers
> without necessitating a black/white list.
>
> If a general check is desirable then my suggestion is to pull out [[
> https://clang.llvm.org/extra/clang-tidy/checks/fuchsia-restrict-system-includes.html
> | fuchsia-restrict-system-includes ]] which already implements the features
> mentioned.
> I've added a check to allow for compiler provided headers without
> necessitating a black/white list.
This is a much better solution for sure.
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/llvmlibc-restrict-system-libc-headers.rst:11-13
+ #include <stdio.h> // Not allowed because it is part of system
libc
+ #include <stddef.h> // Allowed because it is provided by the
compiler
+ #include "internal/stdio.h" // Allowed because it is NOT part of system
libc
----------------
Nit: We do periods at the end of comments in real code so for parity I think it
makes sense here too.
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/llvmlibc-restrict-system-libc-headers.rst:16-20
+This check is necesary because accidentally including sytem libc headers can
+lead to subtle and hard to detect bugs. For example consider a system libc
+whose `FILE *` struct has slightly different field ordering than llvm-libc.
+While this will compile successfully, this can cause issues during runtime
+because they are ABI incompatible.
----------------
To be annoyingly pedantic, `FILE` is opaque so it would actually be ok in this
situation. It is undefined to allocate your own `FILE` I believe. Something
like `jmp_buf` or `thrd_t` might be better examples? Or macros like `assert` or
`errno`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75332/new/
https://reviews.llvm.org/D75332
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
