[PATCH] D43778: [clang-tidy] Adding RestrictIncludes check to Fuchsia module

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/fuchsia/RestrictSystemIncludesCheck.cpp:67
+  llvm::SmallDenseMap<FileID, FileIncludes> IncludeDirectives;
+  llvm::StringMap<bool> IsSystem;
+
----------------
Rather than use this `StringMap`, can you change `InclusionDirective()` to filter out includes you don't care about? Something along the lines of:
```
FileID FID = SM.translateFile(File);
assert(FID != FileID() && "Source Manager does not know about this header file");

bool Invalid;
const SLocEntry &Entry = SM.getSLocEntry(FID, &Invalid);
if (!Invalid && SrcMgr::isSystem(Entry.getFile().getFileCharacteristic())) {
  // It's a system file.
}
```
This way you don't have to store all the path information and test against paths in `EndOfMainFile()`, unless I'm missing something. Note, this is untested code and perhaps that assert will fire (maybe SourceManager is unaware of this file by the time InclusionDirective() is called).

Alternatively, you could alter InclusionDirective() to also pass in the `CharacteristicKind` calculated by `Preprocessor::HandleIncludeDirective()` as that seems like generally useful information for the callback to have on hand.


================
Comment at: clang-tidy/fuchsia/RestrictSystemIncludesCheck.h:1-2
+//===--- RestrictSystemIncludesCheck.h - clang-tidy---------------------*-
+//C++-*-===//
+//
----------------
This should only be on a single line --  you can remove some `-` characters.


================
Comment at: docs/clang-tidy/checks/fuchsia-restrict-system-includes.rst:32
+   A string containing a semi-colon separated list of allowed include filenames.
+   The default is an empty string, which allows all includes.
----------------
This default seems a bit odd to me, but perhaps it's fine. What's novel is that the check is a no-op by default, so how do Fuchsia developers get the correct list? Or is there no canonical list and developers are expected to populate their own manually?


https://reviews.llvm.org/D43778