[PATCH] D15710: [clang-tidy] Add non-inline function definition and variable definition check in header files.

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.

================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:26
@@ +25,3 @@
+  StringRef Filename = SM.getFilename(ExpansionLoc);
+  return Filename.endswith(".h") || Filename.endswith(".hh") ||
+         Filename.endswith(".hpp") || Filename.endswith(".hxx");
----------------
>Having to configure this in a single place would be convenient. OTOH, I can imagine corner-cases, where a single option would be undesired. One thing we could do is to allow global options that can be overridden for each check. Something along the lines of (modulo tests):
<snip>
> Alternatively, we could add another method (e.g. getLocalOrGlobal) to make the usage of the global setting explicit.
> 
> What do you think?

I think that's a good idea, and definitely agree it can be done in a follow-up patch.

================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:27
@@ +26,3 @@
+  return Filename.endswith(".h") || Filename.endswith(".hh") ||
+         Filename.endswith(".hpp") || Filename.endswith(".hxx");
+}
----------------
I think we should also have a case for an extension-less file here (for people who write headers like <string>).

================
Comment at: docs/clang-tidy/checks/misc-definitions-in-headers.rst:20
@@ +19,3 @@
+   const int c = 1;
+   namespace {
+     int f = 2;
----------------
hokein wrote:
> Done. I have also updated my comments here for these cases. Right now it should be ready for review again. Thanks.
> I suggest that we ignore these for now and maybe add an option later.

For the static and const declarations I can see some benefit to ignoring for now, but the unnamed namespace in a header file is sufficiently nasty that many coding guidelines explicitly forbid it because of ODR violations (it really doesn't do what users think it will do). It's especially strange that we will warn about using a *named* namespace in a header file, but not an *unnamed* namespace, which is decidedly more behaviorally strange. I would prefer to see the check catch at least unnamed namespaces, if not static or const variable declarations.

Also, I would be interested in hearing what "a lot of such usages in Google" means in more concrete terms for static or const definitions in header files, if you're able to share those numbers. I don't foresee that changing the requirements for this patch, but I'd still be curious to know for when I work on CERT's secure coding rules.


http://reviews.llvm.org/D15710