aaron.ballman added a subscriber: aaron.ballman.
================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:22
@@ +21,3 @@
+
+bool inHeaderFile(const SourceManager* SM, SourceLocation Location) {
+ StringRef Filename = SM->getFilename(SM->getExpansionLoc(Location));
----------------
This should use isInMainFile() instead of checking the file extension.
================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:30
@@ +29,3 @@
+void DefinitionsInHeadersCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(
+ decl(anyOf(functionDecl(isDefinition()),
----------------
I wonder if you could use an AST matcher to weed out definitions that are in
the main file to make the matcher a bit more narrow.
================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:36
@@ +35,3 @@
+void DefinitionsInHeadersCheck::check(const MatchFinder::MatchResult &Result) {
+ // C++ [basic.def.odr]:
+ // There can be more than one definition of a class type, enumeration type,
----------------
Please quote the paragraph as well (p6, in this case).
================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:44
@@ +43,3 @@
+ // satisfy the following requirements.
+ const auto* ND = Result.Nodes.getNodeAs<NamedDecl>("decl");
+ if (!ND)
----------------
Should be declared: `const auto *ND` (note the position of the *). May want to
run clang-format over your patch, as this applies elsewhere as well.
================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:49
@@ +48,3 @@
+ return;
+ // Internal linkage variable and function definitions are allowed:
+ // const int a = 1;
----------------
Why are these allowed? They can result in ODR violations if they are in a
header file.
================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:69
@@ +68,3 @@
+ return;
+ // Member function of a class template and member function of a nest class
+ // in a class template are allowed.
----------------
nest -> nested
================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:77
@@ +76,3 @@
+ return;
+ DC = DC->getLookupParent();
+ }
----------------
Why the lookup parent instead of getParent()? Also, can this ever return a
nullptr?
================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:85
@@ +84,3 @@
+ } else if (const auto* VD = dyn_cast<VarDecl>(ND)) {
+ // static data member of a class template is allowed.
+ if (VD->getDeclContext()->isDependentContext() &&
----------------
static -> Static
================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.h:19
@@ +18,3 @@
+
+// Finds non-extern non-inline function dand variable definitions in header
+// files, which can lead to potential ODR violations.
----------------
dand -> and
================
Comment at: docs/clang-tidy/checks/misc-definitions-in-headers.rst:17
@@ +16,3 @@
+ namespace {
+ int c = 2; // ok
+ }
----------------
I don't think that this should be okay (same with static/const int examples
above). See
https://www.securecoding.cert.org/confluence/display/cplusplus/DCL59-CPP.+Do+not+define+an+unnamed+namespace+in+a+header+file
for more details.
================
Comment at: unittests/clang-tidy/MiscModuleTest.cpp:38
@@ -36,1 +37,3 @@
+class DefinitionsInHeadersCheckTest : public ::testing::Test {
+protected:
----------------
Is there a reason this is under unittests instead of as actual clang-tidy test
files? I would prefer to see this run through clang-tidy on the shell instead
of in the unit tests, unless there's something we cannot otherwise test.
http://reviews.llvm.org/D15710
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
