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

[Paula Toth via Phabricator via cfe-commits]

PaulkaToast updated this revision to Diff 248851.
PaulkaToast added a comment.

Thanks for the suggestions, the general check sounds like a great idea. I can see the use case for this as it can be used by anyone. I took the time to port out fuchsia's check and flesh out the user facing documentation. Here is the patch for that D75786 <https://reviews.llvm.org/D75786>.

Keeping that in mind, I believe using the general check with a list of headers in llvm-libc's case is a bad idea due the following edge cases:

1. The compiler stops providing an include
------------------------------------------

If we had a list that specifically allowed stdbool.h and imagine the compiler used stopped providing this header, then we may accidentally pick up the system stdbool.h. Having to change the `.clang-tidy` file in llvm-libc's tree when the compiler provided includes changes is a maintenance burden and can quickly become stale.

2. Platform differences
-----------------------

The compiler provided headers may not be the same from operating system to operating system. This introduces the need for multiple lists for each system supported, where each list suffers from the above problem.

3. Accidental changes to the include order
------------------------------------------

In situations where a mistake is made in the build system and higher priority is given to the system includes over the compiler includes. A hand maintained list would not be able to catch this.

---

Above all else this test needs to be as strict as possible since many of these situations would allow libc to continue to compile, maybe even pass the tests but at run-time it could introduce bad behavior due to possible differences in implementation details. It is important to us to not have a human-point of failure on this check in my opinion.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75332/new/

https://reviews.llvm.org/D75332

Files:
  clang-tools-extra/clang-tidy/CMakeLists.txt
  clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
  clang-tools-extra/clang-tidy/llvmlibc/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvmlibc/LLVMLibcTidyModule.cpp
  clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp
  clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/llvmlibc-restrict-system-libc-headers.rst
  clang-tools-extra/docs/clang-tidy/index.rst
  clang-tools-extra/test/clang-tidy/checkers/Inputs/llvmlibc/transitive.h
  clang-tools-extra/test/clang-tidy/checkers/llvmlibc-restrict-system-libc-headers-transitive.cpp
  clang-tools-extra/test/clang-tidy/checkers/llvmlibc-restrict-system-libc-headers.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D75332.248851.patch
Type: text/x-patch
Size: 13401 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200307/577f9ef1/attachment-0001.bin>