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

[Julie Hockett via Phabricator via cfe-commits]

juliehockett added inline comments.


================
Comment at: clang-tidy/fuchsia/RestrictIncludesCheck.cpp:75
+    for (const auto &Include : FileDirectives) {
+      auto D = Check.diag(Include.Loc, "restricted include found");
+
----------------
aaron.ballman wrote:
> I believe this check will be finding transitive includes, correct? If so, maybe this should be two diagnostics: one to say `"'%0' is prohibited from being included"` and a note for the transitive case to say `"file included transitively from here"`?
> 
> Regardless of the behavior you'd like for it, we should have a test case:
> ```
> // a.h
> // Assumed that a.h is prohibited
> void f();
> 
> // b.h
> #include "a.h"
> 
> // c.c
> #include "b.h"
> 
> int main() {
>   f();
> }
> ```
It will flag, but not fix, if a disallowed file is transitively included *and* the relevant flag is passed (e.g. -system-headers for system, or -header-filter=.* for other. I changed the warning text for header warnings to clarify that.


================
Comment at: clang-tidy/fuchsia/RestrictIncludesCheck.cpp:83
+
+      D << FixItHint::CreateRemoval(ToRange);
+    }
----------------
aaron.ballman wrote:
> Are you okay with breaking user's code from this removal? If you remove the header file, but not the parts of the code that require that header file to be included, this FixIt will break code.
Yes, but I did add a note to the documentation to call that out.


================
Comment at: docs/clang-tidy/checks/fuchsia-restrict-includes.rst:27
+
+   A string containing a comma-separated list of header filenames to restrict. Default is an empty string.
----------------
hokein wrote:
> The check seems do nothing with the default option.
> 
> Do we have a corresponding guideline of fuchsia? 
The idea is to maintain a list of disallowed headers in the local .clang-tidy file, so it will be dependent on that. If no headers are restricted, the check won't do anything. I'll add documentation to that effect.


https://reviews.llvm.org/D43778