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