[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:37-39
+    IncludeDirective(SourceLocation Loc_, CharSourceRange Range_,
+                     StringRef Filename_)
+        : Loc(Loc_), Range(Range_), Filename(Filename_) {}
----------------
Please don't suffix with an underscore -- you can actually drop the underscore here (we allow constructor arguments to hide the names of class members).


================
Comment at: clang-tidy/fuchsia/RestrictIncludesCheck.cpp:48-49
+
+  typedef std::vector<IncludeDirective> FileIncludes;
+  std::map<clang::FileID, FileIncludes> IncludeDirectives;
+
----------------
Eugene.Zelenko wrote:
> Please use using and run Clang-tidy modernize.
Are you sure that `vector` and `map` are the best data types to use, rather than LLVM ADTs?

Also you can drop the elaborated type specifier for `FileID`.


================
Comment at: clang-tidy/fuchsia/RestrictIncludesCheck.cpp:69
+
+  for (auto &Bucket : IncludeDirectives) {
+    auto &FileDirectives = Bucket.second;
----------------
`const auto &`


================
Comment at: clang-tidy/fuchsia/RestrictIncludesCheck.cpp:74
+    for (const auto &Include : FileDirectives) {
+      // Emit a warning.
+      auto D = Check.diag(Include.Loc, "Restricted include found.");
----------------
This comment doesn't really add much value.


================
Comment at: clang-tidy/fuchsia/RestrictIncludesCheck.cpp:75
+      // Emit a warning.
+      auto D = Check.diag(Include.Loc, "Restricted include found.");
+
----------------
Diagnostics are not complete sentences, so you should remove the punctuation and capitalization here.


================
Comment at: clang-tidy/fuchsia/RestrictIncludesCheck.h:32
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  std::vector<std::string> &getRestrictedIncludes() {
+    return RestrictedIncludes;
----------------
Function can be `const` and return a `const` ref.


================
Comment at: test/clang-tidy/fuchsia-restrict-includes.cpp:13
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: Restricted include found.
+// CHECK-FIXES-NOT: #include <s.h>
----------------
I think this could use some edge case tests for awful things people can do:
```
#define foo <stdio.h>

#include foo

# /* hahaha */ include /* oops */ foo
```


https://reviews.llvm.org/D43778