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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
