alexfh added inline comments.
================
Comment at: clang-tidy/google/DefinitionsInHeadersCheck.cpp:23
@@ +22,3 @@
+bool inHeaderFile(const SourceManager* SM,
+ const SourceLocation& Location) {
+ StringRef Filename = SM->getFilename(SM->getExpansionLoc(Location));
----------------
`SourceLocations` are small and trivially-copyable, they should be passed by
value.
================
Comment at: clang-tidy/google/DefinitionsInHeadersCheck.cpp:31
@@ +30,3 @@
+void DefinitionsInHeadersCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(functionDecl(isDefinition()).bind("fun-definition"),
this);
+ Finder->addMatcher(varDecl(isDefinition()).bind("var-definition"), this);
----------------
I'd use a single matcher for both functions and variables
(`decl(anyOf(functionDecl(), varDecl()), isDefinition()).bind("decl")`) to
avoid duplicating code in the callback.
================
Comment at: clang-tidy/google/DefinitionsInHeadersCheck.cpp:42
@@ +41,3 @@
+ // appears in a different translation unit, and provided the definitions
+ // satisfy the following requirements. (C++ basic.def.odr)
+ if (auto funDecl = Result.Nodes.getNodeAs<FunctionDecl>("fun-definition")) {
----------------
Please put the `C++ [basic.def.odr]:` part before the quotation from the
standard.
================
Comment at: clang-tidy/google/DefinitionsInHeadersCheck.cpp:43
@@ +42,3 @@
+ // satisfy the following requirements. (C++ basic.def.odr)
+ if (auto funDecl = Result.Nodes.getNodeAs<FunctionDecl>("fun-definition")) {
+ if (!inHeaderFile(Result.SourceManager, funDecl->getLocStart()))
----------------
Please use `const auto *` to make it clear that this is a pointer. Same
elsewhere.
================
Comment at: clang-tidy/google/DefinitionsInHeadersCheck.cpp:52
@@ +51,3 @@
+ // Inline function is allowed.
+ if (funDecl->isInlined())
+ return;
----------------
This check can be done in the matcher.
================
Comment at: clang-tidy/google/DefinitionsInHeadersCheck.cpp:62
@@ +61,3 @@
+ if (auto cxxMethodDecl = dyn_cast<CXXMethodDecl>(funDecl)) {
+ if (cxxMethodDecl->getParent()->getDescribedClassTemplate()) {
+ return;
----------------
nit: No braces needed here.
================
Comment at: clang-tidy/google/DefinitionsInHeadersCheck.cpp:68
@@ +67,3 @@
+ diag(funDecl->getLocation(),
+ "function definition is not allowed in header file.")
+ << FixItHint::CreateInsertion(funDecl->getSourceRange().getBegin(),
----------------
Warning messages are not full sentences. No trailing period is needed.
================
Comment at: clang-tidy/google/DefinitionsInHeadersCheck.h:17
@@ +16,3 @@
+namespace tidy {
+namespace google {
+
----------------
This check is generic enough. It should go to the `misc` module, IMO.
================
Comment at: clang-tidy/google/DefinitionsInHeadersCheck.h:19
@@ +18,3 @@
+
+// Finds non-inline function definition and variable definition in headers,
+// which may violate cpp one definition rule.
----------------
This would read slightly better: "Finds non-extern non-inline function and
variable definitions in header files, which can lead to ODR violations."
================
Comment at: clang-tidy/google/DefinitionsInHeadersCheck.h:20
@@ +19,3 @@
+// Finds non-inline function definition and variable definition in headers,
+// which may violate cpp one definition rule.
+class DefinitionsInHeadersCheck : public ClangTidyCheck {
----------------
Please add a link to the user-facing documentation:
// For the user-facing documentation see:
// http://clang.llvm.org/extra/clang-tidy/checks/<check-name>.html
================
Comment at: docs/clang-tidy/checks/google-definitions-in-headers.rst:4
@@ +3,3 @@
+
+Finds non-inline function definition and variable definition in headers, which
+may violate cpp one definition rule.
----------------
Please update the wording to match the class comment, and also add a couple of
examples of what cases does the check detect.
================
Comment at: unittests/clang-tidy/GoogleModuleTest.cpp:140
@@ +139,3 @@
+ "}", "foo.h"));
+ EXPECT_EQ(errorMessage, runCheckOnCode("template <typename T>\n"
+ "T f() {\n"
----------------
I think, we can already use raw string literals (at least, MSVC 2013 seems to
support them). Raw string literals would make the test cases more readable, imo.
Repository:
rL LLVM
http://reviews.llvm.org/D15710
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
