[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/RestrictIncludesCheck.cpp:42-44
+    SourceLocation Loc;     ///< '#' location in the include directive
+    CharSourceRange Range;  ///< SourceRange for the file name
+    std::string Filename;   ///< Filename as a string
----------------
We don't really use Doxygen comments for local class stuff, so you can convert these into regular comments.


================
Comment at: clang-tidy/fuchsia/RestrictIncludesCheck.cpp:75
+    for (const auto &Include : FileDirectives) {
+      auto D = Check.diag(Include.Loc, "restricted include found");
+
----------------
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();
}
```


================
Comment at: clang-tidy/fuchsia/RestrictIncludesCheck.cpp:83
+
+      D << FixItHint::CreateRemoval(ToRange);
+    }
----------------
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.


================
Comment at: clang-tidy/fuchsia/RestrictIncludesCheck.cpp:87
+
+  IncludeDirectives.clear();
+}
----------------
Do you need to clear this? I don't think `EndOfMainFile()` can be called twice, can it?


================
Comment at: clang-tidy/fuchsia/RestrictIncludesCheck.cpp:92-95
+  for (const auto &Include : Check.getRestrictedIncludes()) {
+    if (IncludeFile == Include) return true;
+  }
+  return false;
----------------
This can be replaced with: `return llvm::is_contained(Check.getRestrictedIncludes(), IncludeFile);` which suggests the private function isn't needed at all.


================
Comment at: clang-tidy/fuchsia/RestrictIncludesCheck.h:20
+
+/// Checks for restricted headers and suggests their removal.
+///
----------------
s/headers/includes


================
Comment at: clang-tidy/fuchsia/RestrictIncludesCheck.h:28
+      : ClangTidyCheck(Name, Context),
+        RestrictedIncludes(utils::options::parseStringList(
+            Options.get("Includes", "::std::vector"))) {}
----------------
There is a mild problem here (present in the previous form, using commas, as well): the separator is technically a legal character for a file name. You may want to provide a comment in the documentation that explicitly states a file with the delimiter in the name is not supported. e.g., a file named `foo.h;bar.h` is not something you can prohibit with this check. I don't think it's worth picking a different delimiter over given how unlikely this file name is in practice.


================
Comment at: clang-tidy/fuchsia/RestrictIncludesCheck.h:29
+        RestrictedIncludes(utils::options::parseStringList(
+            Options.get("Includes", "::std::vector"))) {}
+
----------------
The default value here seems very strange as it's not an include file. I assume this intended to use `vector`?


================
Comment at: docs/clang-tidy/checks/fuchsia-restrict-includes.rst:28
+
+   A string containing a comma-separated list of include filenames to restrict. Default is an empty string.
----------------
The list is not comma-separated, it's semicolon-separated.


https://reviews.llvm.org/D43778