[PATCH] D75332: [clang-tidy] Add module for llvm-libc and restrict-system-libc-header-check.

[Alex Brachet via Phabricator via cfe-commits]

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