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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp:52
+    if (!SM.isInMainFile(HashLoc)) {
+      DiagnosticBuilder D = Check.diag(
+          HashLoc,
----------------
No need for the local DiagnosticBuilder object, you can stream into the Check.diag() call.


================
Comment at: clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp:57
+    } else {
+      DiagnosticBuilder D =
+          Check.diag(HashLoc, "system libc header %0 not allowed");
----------------
No need for the local `DiagnosticBuilder` object, you can stream into the `Check.diag()` call.


================
Comment at: clang-tools-extra/clang-tidy/llvmlibc/RestrictSystemLibcHeadersCheck.cpp:66-67
+    const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
+  SmallString<128> CompilerIncudeDir =
+      StringRef(PP->getHeaderSearchInfo().getHeaderSearchOpts().ResourceDir);
+  llvm::sys::path::append(CompilerIncudeDir, "include");
----------------
The user can control this path -- is that an issue? You're using it to determine what a compiler-provided header file is, and this seems like an escape hatch for users to get around that. If that's reasonable to you, then I'm okay with it, but you had mentioned you want to remove human error as a factor and this seems like it could be a subtle human error situation.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/llvmlibc-restrict-system-libc-headers.rst:16
+
+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
----------------
necesary -> necessary


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75332