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

[hokein]

Summary

The new check will find all functionand variable definitions which may violate cpp one definition rule in header file.

[alexfh]

clang-tidy/google/DefinitionsInHeadersCheck.cpp

23 ↗

(On Diff #43434)

SourceLocations are small and trivially-copyable, they should be passed by value.

31 ↗

(On Diff #43434)

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.

42 ↗

(On Diff #43434)

Please put the C++ [basic.def.odr]: part before the quotation from the standard.

43 ↗

(On Diff #43434)

Please use const auto * to make it clear that this is a pointer. Same elsewhere.

52 ↗

(On Diff #43434)

This check can be done in the matcher.

62 ↗

(On Diff #43434)

nit: No braces needed here.

68 ↗

(On Diff #43434)

Warning messages are not full sentences. No trailing period is needed.

clang-tidy/google/DefinitionsInHeadersCheck.h

17 ↗

(On Diff #43434)

This check is generic enough. It should go to the misc module, IMO.

19 ↗

(On Diff #43434)

This would read slightly better: "Finds non-extern non-inline function and variable definitions in header files, which can lead to ODR violations."

20 ↗

(On Diff #43434)

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

docs/clang-tidy/checks/google-definitions-in-headers.rst

4 ↗

(On Diff #43434)

Please update the wording to match the class comment, and also add a couple of examples of what cases does the check detect.

unittests/clang-tidy/GoogleModuleTest.cpp

140 ↗

(On Diff #43434)

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.

[hokein]

Update patch to address review comments.

[hokein]

clang-tidy/google/DefinitionsInHeadersCheck.cpp

52 ↗

(On Diff #43434)

The isInline AST matcher seems only match the function with inline key words. It could not detect the member function defined within class, such as:

class A {
  int f() { return 1; }
};

clang-tidy/google/DefinitionsInHeadersCheck.h

17 ↗

(On Diff #43434)

Done. Move to misc module

unittests/clang-tidy/GoogleModuleTest.cpp

140 ↗

(On Diff #43434)

The Raw string literals doesn't work well with multiple lines in macro. I try it on my local machine, it will break compilation.

[hokein]

@alexfh The case of member function of a nest class in a class template (like int A<T>::B::fun() {...}) is also fixed now. Please review the patch again. Thanks.

[aaron.ballman]

clang-tidy/misc/DefinitionsInHeadersCheck.cpp

22 ↗

(On Diff #43544)

This should use isInMainFile() instead of checking the file extension.

30 ↗

(On Diff #43544)

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.

36 ↗

(On Diff #43544)

Please quote the paragraph as well (p6, in this case).

44 ↗

(On Diff #43544)

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.

49 ↗

(On Diff #43544)

Why are these allowed? They can result in ODR violations if they are in a header file.

69 ↗

(On Diff #43544)

nest -> nested

77 ↗

(On Diff #43544)

Why the lookup parent instead of getParent()? Also, can this ever return a nullptr?

85 ↗

(On Diff #43544)

static -> Static

clang-tidy/misc/DefinitionsInHeadersCheck.h

19 ↗

(On Diff #43544)

dand -> and

docs/clang-tidy/checks/misc-definitions-in-headers.rst

17 ↗

(On Diff #43544)

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.

unittests/clang-tidy/MiscModuleTest.cpp

38 ↗

(On Diff #43544)

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.

[hokein]

Address aaron's comments.

[hokein]

docs/clang-tidy/checks/misc-definitions-in-headers.rst

18 ↗

(On Diff #43686)

Yeah, it's not a good practice to define an an unnamed namespace in header file (Google cpp code style also suggests that).

But I think internal linkage variables are ok here, the variable only be visible in current translation unit which will not cause ODR violation. And It's common to define a static const constant in C++ header file.

BTW, the sample code in the link can be compiled in g++ v4.8

unittests/clang-tidy/MiscModuleTest.cpp

38 ↗

(On Diff #43686)

I'm not sure about this. The reason I put the test in unittest is that I see the other header checks(GlobalNamesInHeadersCheck) are also in the unittest. It would be better to keep the consistency.

[aaron.ballman]

docs/clang-tidy/checks/misc-definitions-in-headers.rst

18 ↗

(On Diff #43686)

They do cause ODR violations. In addition to the CERT link above, see http://stackoverflow.com/questions/23652156/how-would-use-of-unnamed-namespaces-in-headers-cause-odr-violations for further information.

unittests/clang-tidy/MiscModuleTest.cpp

38 ↗

(On Diff #43686)

I was arguing for consistency as well -- clang-tidy tests are run via lit instead of unit tests unless there's a reason to do otherwise. It sounds like there's no reason for these to be unit tests.

[alexfh]

clang-tidy/google/DefinitionsInHeadersCheck.cpp

52 ↗

(On Diff #43434)

Yep, I've found this out myself, but have forgotten to remove the comment.

clang-tidy/misc/DefinitionsInHeadersCheck.cpp

23 ↗

(On Diff #43686)

I don't think "main file" is the right condition here. If we run clang-tidy on a header file, it will be the main file, but we still are interested in the diagnostics in it. So I don't see a better way to distinguish header files than looking at their extension (some projects might need the list of extensions to be configurable, but that's another story).

31 ↗

(On Diff #43686)

Do you expect this to improve performance?

47 ↗

(On Diff #43686)

I'd replace this with an assertion, since this bound node should always be a NamedDecl. To make this explicit, I'd also use the namedDecl matcher instead of decl above.

53 ↗

(On Diff #43686)

nit: Use of the same variable name distracts from the essence of the example. Please use unique variable names.

56 ↗

(On Diff #43686)

nit: Missing ; after 1.

docs/clang-tidy/checks/misc-definitions-in-headers.rst

2 ↗

(On Diff #43686)

nit: Please remove two ='s.

unittests/clang-tidy/GoogleModuleTest.cpp

140 ↗

(On Diff #43434)

You mean, the EXPECT_EQ macro doesn't work with raw string literals somehow? That sounds strange.

unittests/clang-tidy/MiscModuleTest.cpp

38 ↗

(On Diff #43686)

I don't feel strongly about this, but I generally prefer lit tests as well, since they allow to test both messages and fixes easily and with less boilerplate. It's true, that setting up a test case with multiple header files may be somewhat easier in a unit test (where everything is described in a single place), but this doesn't seem to be the case.

Once you start testing fixits, a lit test will immediately become a simpler solution.

[aaron.ballman]

clang-tidy/misc/DefinitionsInHeadersCheck.cpp

23 ↗

(On Diff #43686)

I would argue that if you run clang-tidy on just the header file, you might know what you are doing and expect to not see those diagnostics at all.

Relying on the extension makes this far less useful, IMO. For instance, even on this second try, the checker will fail to catch ODR violations in STL-style header files. You can #include "file.cpp" just as easily as "file.h" (as we do with .inc files, for instance), and that can still result in ODR violations too. To me, the extension isn't what makes this checker do interesting work, it's the fact that these can only be violations when the file is included in multiple TUs (except we can only check one TU at a time, so looking for the file to be included is the best we can do).

31 ↗

(On Diff #43686)

Possibly improve performance (I would suspect there are a lot of varDecls and a fair number of functionDecls in the main file), but I also prefer the checkers to be as declarative as possible. It makes it easier (at least for me) to reason about what AST nodes the check is ultimately interested in.

[alexfh]

clang-tidy/misc/DefinitionsInHeadersCheck.cpp

23 ↗

(On Diff #43686)

I would argue that if you run clang-tidy on just the header file, you might know what you are doing and expect to not see those diagnostics at all.

I don't agree here. What one runs the check on is a purely technical detail. It may be even hidden from the user by tooling (e.g. editor or code review integration).

Relying on the extension makes this far less useful, IMO. For instance, even
on this second try, the checker will fail to catch ODR violations in STL-style
header files.

We can make it work for STL-style header files by treating extensionless files as headers. Or make this filter a regular expression or something like that.

You can #include "file.cpp" just as easily as "file.h" (as we do with .inc files, for instance), and that can still result in ODR violations too.

We don't usually include .cpp files. And the .inc extension says the file is a textual include and usually can't be parsed on its own. And it's very uncommon to actually compile .h files on their own as a part of the build. So I'm inclined to treat the file extension as a valuable piece of information regarding its intended use.

To me, the extension isn't what makes this checker do interesting work, it's the
fact that these can only be violations when the file is included in multiple TUs

If someone includes a ".cpp" file, it's a problem, but, IMO, detection of actually present ODR violations is a task for a different tool (possibly, another clang-tidy check, once we have multi-translation-unit support). This check's purpose is to detect a certain class of issues that can potentially cause ODR violations.

(except we can only check one TU at a time, so looking for the file to be
included is the best we can do).

We can do even better by utilizing the information we already have in the file name. Do you see any specific issues that can result from this?

31 ↗

(On Diff #43686)

As noted above, "main file" is not a useful condition here. However, it's certainly possible to move some of the checks from the check method to matcher(s) (e.g. isLocationInHeaderFile). I don't feel strongly about that.

[aaron.ballman]

clang-tidy/misc/DefinitionsInHeadersCheck.cpp

23 ↗

(On Diff #43686)

You can #include "file.cpp" just as easily as "file.h" (as we do with .inc files, for instance), and that can still result in ODR violations too.

We don't usually include .cpp files. And the .inc extension says the file is a textual include and usually can't be parsed on its own. And it's very uncommon to actually compile .h files on their own as a part of the build. So I'm inclined to treat the file extension as a valuable piece of information regarding its intended use.

The point I was getting at is that an extension is arbitrary, though somewhat conventionally-named. I don't see it as being particularly valuable in this case because "has this file been included" is sufficient information to determine whether we want to diagnose or not. I guess that these arguments all boil down to which is more likely to happen: a header file with an extension we don't capture (because user's aren't going to *know* to customize the header file extension list, generally), or the user running this checker on just the header file, but never a source file that includes the header file. While I can think of cases where the user runs it on just the header file, they seem few and far between.

Perhaps another solution to this is use isInMainFile() || usesHeaderFileExtension().

We can do even better by utilizing the information we already have in the file name. Do you see any specific issues that can result from this?

The file extension is never going to be sufficient, or even accurate, information because the extensions are just conventions and not everyone follows the typical ones. For instance, you said that .inc files usually cannot stand on their own, but that isn't strictly followed even in our project (some of the table-generated source files are .inc files that stand alone). Since this check's predominate purpose is to prevent possible ODR violations, inclusion gives more pertinent information, IMO.

[hokein]

Move unittest to lit test.

[hokein]

clang-tidy/misc/DefinitionsInHeadersCheck.cpp

31 ↗

(On Diff #43826)

@aaron, what do you mean the AST matcher here? Could you explain it a bit more?

[aaron.ballman]

clang-tidy/misc/DefinitionsInHeadersCheck.cpp

31 ↗

(On Diff #43826)

Currently, your code checks whether something is in the header file as part of the check() callback and early returns out of it; I was suggesting making the header file check part of the AST matcher in registerMatchers() instead, so that check() isn't called on declarations that are outside of a header file at all. This makes the matcher declaratively more accurate, and it could have a (likely minor) positive performance impact.

[hokein]

Move header file check to AST matcher.

[alexfh]

clang-tidy/misc/DefinitionsInHeadersCheck.cpp

24 ↗

(On Diff #43826)

We're looking at the problem from different angles. My view is that a reasonable file naming convention (which at least makes interface header files, textual headers and main files distinguishable) is a widespread enough practice, and the benefits it brings outweigh the costs of enforcing it. However, the opposite point of view also has its right to exist, so we need a solution that fits both ;)

Perhaps another solution to this is use isInMainFile() || usesHeaderFileExtension().

You probably meant !isInMainFile() || usesHeaderFileExtension(). I guess, that will work for us. We could also make the list of header file extensions (or a regular expression pattern for header files) configurable, so that the usesHeaderFileExtension() part could be disabled, if needed.

[aaron.ballman]

clang-tidy/misc/DefinitionsInHeadersCheck.cpp

25 ↗

(On Diff #43886)

We're looking at the problem from different angles. My view is that a reasonable file naming convention (which at least makes interface header files, textual headers and main files distinguishable) is a widespread enough practice, and the benefits it brings outweigh the costs of enforcing it. However, the opposite point of view also has its right to exist, so we need a solution that fits both ;)

Perhaps another solution to this is use isInMainFile() || usesHeaderFileExtension().

You probably meant !isInMainFile() || usesHeaderFileExtension(). I guess, that will work for us. We could also make the list of header file extensions (or a regular expression pattern for header files) configurable, so that the usesHeaderFileExtension() part could be disabled, if needed.

Oops, you are correct, I meant !isInMainFile(). :-) I definitely agree that we should make the header file extensions configurable. Would it make sense if this were a global option that any checker can use? We have 3-4 other checkers that care about header files as well, and it would be nice if they all behaved consistently without the user having to write a lot of options for each checker.

[hokein]

clang-tidy/misc/DefinitionsInHeadersCheck.cpp

25 ↗

(On Diff #43886)

I'm :+1 on making header file extensions configurable. I think we can do that in a new patch.

[alexfh]

clang-tidy/misc/DefinitionsInHeadersCheck.cpp

25 ↗

(On Diff #43886)

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):

  std::string OptionsView::get(StringRef LocalName, std::string Default) const {
-   const auto &Iter = CheckOptions.find(NamePrefix + LocalName.str());
+   auto Iter = CheckOptions.find(NamePrefix + LocalName.str());
    if (Iter != CheckOptions.end())
      return Iter->second;
+   // Fallback to global setting, if present.
+   Iter = CheckOptions.find(LocalName.str());
+   if (Iter != CheckOptions.end())
+     return Iter->second;
    return Default;
  }

Alternatively, we could add another method (e.g. getLocalOrGlobal) to make the usage of the global setting explicit.

What do you think?

25 ↗

(On Diff #43886)

This should read "Having a way to configure this in a single place ..."

[alexfh]

clang-tidy/misc/DefinitionsInHeadersCheck.cpp

25 ↗

(On Diff #43886)

I'm :+1 on making header file extensions configurable. I think we can do that in a new patch.

Yep, this is fine for a follow-up.

[hokein]

Add !inMainFile check.

[hokein]

docs/clang-tidy/checks/misc-definitions-in-headers.rst

19 ↗

(On Diff #43887)

About the internal linkage variable definition, how about only warning the variable in unnamed namespace?

The const/static variable definitions in header are used more widely (We also find there are a lot of such usages in Google code repo), and Google C++ code style doesn't explicitly give suggestions about such const/static usages.

[alexfh]

clang-tidy/misc/DefinitionsInHeadersCheck.cpp

52 ↗

(On Diff #43887)

I'd say "ignored for now" instead of "allowed". We might want to flag them in the future, but these are used quite frequently, so we don't warn on these now to keep the number of warnings under control.

Maybe there should be an option for these cases.

docs/clang-tidy/checks/misc-definitions-in-headers.rst

8 ↗

(On Diff #43887)

nit: It's a warning, not error. Also, please use proper capitalization and punctuation:

int a = 1; // Warning.

or maybe even include the message:

int a = 1; // Warning: "variable definition is not allowed in header file".

Same holds for // ok: They should either be // OK. or maybe explain why certain patterns don't generate a warning.

test/clang-tidy/misc-definitions-in-headers.hpp

10 ↗

(On Diff #43887)

The // ok comments add no new information. Either explain why are these OK, or just remove the comments.

27 ↗

(On Diff #43887)

We usually leave each distinct warning message full once and truncate the other CHECK-MESSAGES patterns to fit to the 80 characters limit (e.g. here I'd remove the in header file [misc-definitions-in-headers] part).

test/lit.cfg

46 ↗

(On Diff #43887)

Please place the new extension after '.cpp' - it seems to be most relevant there.

[alexfh]

docs/clang-tidy/checks/misc-definitions-in-headers.rst

19 ↗

(On Diff #43887)

I suggest that we ignore these for now and maybe add an option later.

[hokein]

Add UseHeaderFileExtension option.

[hokein]

Update doc.

[hokein]

Update doc and address comments on test file.

[hokein]

Update.

[hokein]

Correct punctuation usage.

[hokein]

docs/clang-tidy/checks/misc-definitions-in-headers.rst

20 ↗

(On Diff #44013)

Done. I have also updated my comments here for these cases. Right now it should be ready for review again. Thanks.

[aaron.ballman]

clang-tidy/misc/DefinitionsInHeadersCheck.cpp

26 ↗

(On Diff #44013)

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.

27 ↗

(On Diff #44013)

I think we should also have a case for an extension-less file here (for people who write headers like <string>).

docs/clang-tidy/checks/misc-definitions-in-headers.rst

20 ↗

(On Diff #44013)

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.

[hokein]

Address Aaron's comments.

[hokein]

docs/clang-tidy/checks/misc-definitions-in-headers.rst

21 ↗

(On Diff #44193)

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.

Done.

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.

Yes, the const/static definitions are used widely in header files. The number is ~200,000 including third-party projects.

[aaron.ballman]

LGTM, thank you for working on this!

docs/clang-tidy/checks/misc-definitions-in-headers.rst

21 ↗

(On Diff #44193)

Yes, the const/static definitions are used widely in header files. The number is ~200,000 including third-party projects.

Yoiks, that is annoyingly high. Thank you for the numbers.

[hokein]

In D15710#321198, @aaron.ballman wrote:

LGTM, thank you for working on this!

Ping @alexfh. After the patch gets merged, I will work on the configuration of header file extension.

[alexfh]

Looks almost good to me. A few more comments though.

clang-tidy/misc/DefinitionsInHeadersCheck.cpp

22 ↗

(On Diff #44193)

nit: This name is not clear to me. Did you mean isHeaderFileExtension or usesHeaderFileExtension?

28 ↗

(On Diff #44193)

I'm going to disappoint you: any string ends with an empty string ;)

You'll need a better logic for comparing extensions. llvm::sys::path::extension() might be useful here.

80 ↗

(On Diff #44193)

nit: I'd say in plural: Inline functions are allowed.. Same below.

115 ↗

(On Diff #44193)

Maybe add the name of the variable to the message? This would help users to understand the cause of the warning easier in case of macros, for example.

Same for the other message.

[alexfh]

clang-tidy/misc/DefinitionsInHeadersCheck.cpp

115 ↗

(On Diff #44193)

Also, we shouldn't say not allowed (not allowed by whom/what? Google C++ style guide? this check is not Google-specific, so it shouldn't rely on the user knowing/caring about Google C++ style). The message doesn't make it clear that variable definitions in header files are dangerous in general, not only this specific definition.

I'd phrase the message along the lines of "variable '%0' defined in a header file; variable definitions in header files can lead to ODR violations".

[hokein]

Address Alex's comments.

[hokein]

clang-tidy/misc/DefinitionsInHeadersCheck.cpp

22 ↗

(On Diff #44193)

IsHeaderFileExtension is more descriptive. Have renamed to it now.

[alexfh]

Looks good with a few nits. Please fix them and I'll submit the patch for you.

Thank you for working on this!

clang-tidy/misc/DefinitionsInHeadersCheck.cpp

23 ↗

(On Diff #44313)

Actually, this name is also confusing, but I don't have a better idea now. Let's leave it like this.

25 ↗

(On Diff #44313)

I think, this should be opposite for two reasons:

1. variable definitions and declarations in system headers are not better than in user headers and can lead to the same problems;
2. there are people who care about system headers, so we need to produce diagnostics on them and let clang-tidy filter them out when not needed.

clang-tidy/misc/DefinitionsInHeadersCheck.h

23 ↗

(On Diff #44313)

nit: add a space before the '('

docs/clang-tidy/checks/misc-definitions-in-headers.rst

15 ↗

(On Diff #44313)

nit: the sentence should end with a period, not a comma.

test/clang-tidy/misc-definitions-in-headers.hpp

4 ↗

(On Diff #44313)

Please specify each distinct warning message (in your case, there are two of them) completely (including the check name) once. Having it in the test (just once) is useful to verify that the message is formatted correctly (in your case, there's no space after the ';', for example).

[hokein]

More updates.

[hokein]

test/clang-tidy/misc-definitions-in-headers.hpp

4 ↗

(On Diff #44313)

Didn't notice it. Done (also add a space after ";").

[alexfh]

test/clang-tidy/misc-definitions-in-headers.hpp

5 ↗

(On Diff #44317)

Now please address the "including the check name" part and I'm happy ;)

[hokein]

Add check name in test.

clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
   AssignOperatorSignatureCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
+  DefinitionsInHeadersCheck.cpp
   InaccurateEraseCheck.cpp
   InefficientAlgorithmCheck.cpp
   MacroParenthesesCheck.cpp
   MacroRepeatedSideEffectsCheck.cpp
   MiscTidyModule.cpp
   MoveConstantArgumentCheck.cpp 
   MoveConstructorInitCheck.cpp
   NewDeleteOverloadsCheck.cpp

clang-tools-extra/trunk/clang-tidy/misc/DefinitionsInHeadersCheck.h

+//===--- DefinitionsInHeadersCheck.h - clang-tidy----------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_DEFINITIONS_IN_HEADERS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_DEFINITIONS_IN_HEADERS_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+// Finds non-extern non-inline function and variable definitions in header
+// files, which can lead to potential ODR violations.
+//
+// There is one option:
+// - `UseHeaderFileExtension`: Whether to use file extension (h, hh, hpp, hxx)
+//   to distinguish header files. True by default.
+//
+// For the user-facing documentation see:
+// http://clang.llvm.org/extra/clang-tidy/checks/misc-definitions-in-headers.html
+class DefinitionsInHeadersCheck : public ClangTidyCheck {
+public:
+  DefinitionsInHeadersCheck(StringRef Name, ClangTidyContext *Context);
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  const bool UseHeaderFileExtension;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_DEFINITIONS_IN_HEADERS_H

clang-tools-extra/trunk/clang-tidy/misc/DefinitionsInHeadersCheck.cpp

+//===--- DefinitionsInHeadersCheck.cpp - clang-tidy------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "DefinitionsInHeadersCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+namespace {
+
+AST_MATCHER(NamedDecl, isHeaderFileExtension) {
+  SourceManager& SM = Finder->getASTContext().getSourceManager();
+  SourceLocation ExpansionLoc = SM.getExpansionLoc(Node.getLocStart());
+  StringRef Filename = SM.getFilename(ExpansionLoc);
+  return Filename.endswith(".h") || Filename.endswith(".hh") ||
+         Filename.endswith(".hpp") || Filename.endswith(".hxx") ||
+         llvm::sys::path::extension(Filename).empty();
+}
+
+} // namespace
+
+DefinitionsInHeadersCheck::DefinitionsInHeadersCheck(
+    StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context),
+        UseHeaderFileExtension(Options.get("UseHeaderFileExtension", true)) {}
+
+void DefinitionsInHeadersCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "UseHeaderFileExtension", UseHeaderFileExtension);
+}
+
+void DefinitionsInHeadersCheck::registerMatchers(MatchFinder *Finder) {
+  if (UseHeaderFileExtension) {
+    Finder->addMatcher(
+        namedDecl(anyOf(functionDecl(isDefinition()), varDecl(isDefinition())),
+                  isHeaderFileExtension()).bind("name-decl"),
+        this);
+  } else {
+    Finder->addMatcher(
+        namedDecl(anyOf(functionDecl(isDefinition()), varDecl(isDefinition())),
+                  anyOf(isHeaderFileExtension(),
+                        unless(isExpansionInMainFile()))).bind("name-decl"),
+        this);
+  }
+}
+
+void DefinitionsInHeadersCheck::check(const MatchFinder::MatchResult &Result) {
+  // C++ [basic.def.odr] p6:
+  // There can be more than one definition of a class type, enumeration type,
+  // inline function with external linkage, class template, non-static function
+  // template, static data member of a class template, member function of a
+  // class template, or template specialization for which some template
+  // parameters are not specifiedin a program provided that each definition
+  // appears in a different translation unit, and provided the definitions
+  // satisfy the following requirements.
+  const auto *ND = Result.Nodes.getNodeAs<NamedDecl>("name-decl");
+  assert(ND);
+
+  // Internal linkage variable definitions are ignored for now:
+  //   const int a = 1;
+  //   static int b = 1;
+  //
+  // Although these might also cause ODR violations, we can be less certain and
+  // should try to keep the false-positive rate down.
+  if (ND->getLinkageInternal() == InternalLinkage)
+    return;
+
+  if (const auto *FD = dyn_cast<FunctionDecl>(ND)) {
+    // Inline functions are allowed.
+    if (FD->isInlined())
+      return;
+    // Function templates are allowed.
+    if (FD->getTemplatedKind() == FunctionDecl::TK_FunctionTemplate)
+      return;
+    // Function template full specialization is prohibited in header file.
+    if (FD->getTemplateSpecializationKind() == TSK_ImplicitInstantiation)
+      return;
+    // Member function of a class template and member function of a nested class
+    // in a class template are allowed.
+    if (const auto *MD = dyn_cast<CXXMethodDecl>(FD)) {
+      const auto *DC = MD->getDeclContext();
+      while (DC->isRecord()) {
+        if (const auto *RD = dyn_cast<CXXRecordDecl>(DC))
+          if (RD->getDescribedClassTemplate())
+            return;
+        DC = DC->getParent();
+      }
+    }
+
+    diag(FD->getLocation(),
+         "function '%0' defined in a header file; "
+         "function definitions in header files can lead to ODR violations")
+        << FD->getNameInfo().getName().getAsString()
+        << FixItHint::CreateInsertion(FD->getSourceRange().getBegin(),
+                                      "inline ");
+  } else if (const auto *VD = dyn_cast<VarDecl>(ND)) {
+    // Static data members of a class template are allowed.
+    if (VD->getDeclContext()->isDependentContext() && VD->isStaticDataMember())
+      return;
+    if (VD->getTemplateSpecializationKind() == TSK_ImplicitInstantiation)
+      return;
+    // Ignore variable definition within function scope.
+    if (VD->hasLocalStorage() || VD->isStaticLocal())
+      return;
+
+    diag(VD->getLocation(),
+         "variable '%0' defined in a header file; "
+         "variable definitions in header files can lead to ODR violations")
+        << VD->getName();
+  }
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp

 //===--- MiscTidyModule.cpp - clang-tidy ----------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "ArgumentCommentCheck.h"
 #include "AssertSideEffectCheck.h"
 #include "AssignOperatorSignatureCheck.h"
 #include "BoolPointerImplicitConversionCheck.h"
+#include "DefinitionsInHeadersCheck.h"
 #include "InaccurateEraseCheck.h"
 #include "InefficientAlgorithmCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MoveConstantArgumentCheck.h"
 #include "MoveConstructorInitCheck.h"
 #include "NewDeleteOverloadsCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ArgumentCommentCheck>("misc-argument-comment");
     CheckFactories.registerCheck<AssertSideEffectCheck>(
         "misc-assert-side-effect");
     CheckFactories.registerCheck<AssignOperatorSignatureCheck>(
         "misc-assign-operator-signature");
     CheckFactories.registerCheck<BoolPointerImplicitConversionCheck>(
         "misc-bool-pointer-implicit-conversion");
+    CheckFactories.registerCheck<DefinitionsInHeadersCheck>(
+        "misc-definitions-in-headers");
     CheckFactories.registerCheck<InaccurateEraseCheck>(
         "misc-inaccurate-erase");
     CheckFactories.registerCheck<InefficientAlgorithmCheck>(
         "misc-inefficient-algorithm");
     CheckFactories.registerCheck<MacroParenthesesCheck>(
         "misc-macro-parentheses");
     CheckFactories.registerCheck<MacroRepeatedSideEffectsCheck>(
         "misc-macro-repeated-side-effects");

clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

.. toctree::
    llvm-header-guard
    llvm-include-order
    llvm-namespace-comment
    llvm-twine-local
    misc-argument-comment
    misc-assert-side-effect
    misc-assign-operator-signature
    misc-bool-pointer-implicit-conversion
+   misc-definitions-in-headers
    misc-inaccurate-erase
    misc-inefficient-algorithm
    misc-macro-parentheses
    misc-macro-repeated-side-effects
    misc-move-constructor-init
    misc-new-delete-overloads
    misc-noexcept-move-constructor
    misc-non-copyable-objects

clang-tools-extra/trunk/docs/clang-tidy/checks/misc-definitions-in-headers.rst

+misc-definitions-in-headers
+===========================
+
+Finds non-extern non-inline function and variable definitions in header files, which can lead to potential ODR violations.
+
+.. code:: c++
+   // Foo.h
+   int a = 1; // Warning.
+   extern int d; // OK: extern variable.
+
+   namespace N {
+     int e = 2; // Warning.
+   }
+
+   // Internal linkage variable definitions are ignored for now.
+   // Although these might also cause ODR violations, we can be less certain and
+   // should try to keep the false-positive rate down.
+   static int b = 1;
+   const int c = 1;
+
+   // Warning.
+   int g() {
+     return 1;
+   }
+
+   // OK: inline function definition.
+   inline int e() {
+     return 1;
+   }
+
+   class A {
+    public:
+     int f1() { return 1; } // OK: inline member function definition.
+     int f2();
+   };
+
+   int A::f2() { return 1; } // Warning.

clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy.py

def main():
   resource_dir = args.resource_dir
   input_file_name = args.input_file_name
   check_name = args.check_name
   temp_file_name = args.temp_file_name
 
   extension = '.cpp'
   if (input_file_name.endswith('.c')):
     extension = '.c'
+  if (input_file_name.endswith('.hpp')):
+    extension = '.hpp'
   temp_file_name = temp_file_name + extension
 
   clang_tidy_extra_args = extra_args
   if len(clang_tidy_extra_args) == 0:
     clang_tidy_extra_args = ['--', '--std=c++11'] if extension == '.cpp' \
                        else ['--']
   if resource_dir is not None:
     clang_tidy_extra_args.append('-resource-dir=%s' % resource_dir)

clang-tools-extra/trunk/test/clang-tidy/misc-definitions-in-headers.hpp

+// RUN: %check_clang_tidy %s misc-definitions-in-headers %t
+
+int f() {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f' defined in a header file; function definitions in header files can lead to ODR violations [misc-definitions-in-headers]
+// CHECK-FIXES: inline int f() {
+  return 1;
+}
+
+class CA {
+  void f1() {} // OK: inline class member function definition.
+  void f2();
+  template<typename T>
+  T f3() {
+    T a = 1;
+    return a;
+  }
+  template<typename T>
+  struct CAA {
+    struct CAB {
+      void f4();
+    };
+  };
+};
+
+void CA::f2() { }
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: function 'f2' defined in a header file;
+// CHECK-FIXES: inline void CA::f2() {
+
+template <>
+int CA::f3() {
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: function 'f3' defined in a header file;
+  int a = 1;
+  return a;
+}
+
+template <typename T>
+void CA::CAA<T>::CAB::f4() {
+// OK: member function definition of a nested template class in a class.
+}
+
+template <typename T>
+struct CB {
+  void f1();
+  struct CCA {
+    void f2(T a);
+  };
+  struct CCB;  // OK: forward declaration.
+  static int a; // OK: class static data member declaration.
+};
+
+template <typename T>
+void CB<T>::f1() { // OK: Member function definition of a class template.
+}
+
+template <typename T>
+void CB<T>::CCA::f2(T a) {
+// OK: member function definition of a nested class in a class template.
+}
+
+template <typename T>
+struct CB<T>::CCB {
+  void f3();
+};
+
+template <typename T>
+void CB<T>::CCB::f3() {
+// OK: member function definition of a nested class in a class template.
+}
+
+template <typename T>
+int CB<T>::a = 2; // OK: static data member definition of a class template.
+
+template <typename T>
+T tf() { // OK: template function definition.
+  T a;
+  return a;
+}
+
+
+namespace NA {
+  int f() { return 1; }
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: function 'f' defined in a header file;
+// CHECK-FIXES: inline int f() { return 1; }
+}
+
+template <typename T>
+T f3() {
+  T a = 1;
+  return a;
+}
+
+template <>
+// CHECK-MESSAGES: :[[@LINE+1]]:5: warning: function 'f3' defined in a header file;
+int f3() {
+  int a = 1;
+  return a;
+}
+
+int f5(); // OK: function declaration.
+inline int f6() { return 1; } // OK: inline function definition.
+namespace {
+  int f7() { return 1; }
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: function 'f7' defined in a header file;
+}
+
+int a = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: variable 'a' defined in a header file; variable definitions in header files can lead to ODR violations [misc-definitions-in-headers]
+CA a1;
+// CHECK-MESSAGES: :[[@LINE-1]]:4: warning: variable 'a1' defined in a header file;
+
+namespace NB {
+  int b = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'b' defined in a header file;
+  const int c = 1; // OK: internal linkage variable definition.
+}
+
+class CC {
+  static int d; // OK: class static data member declaration.
+};
+
+int CC::d = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: variable 'd' defined in a header file;
+
+const char* ca = "foo";
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: variable 'ca' defined in a header file;
+
+namespace {
+  int e = 2;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'e' defined in a header file;
+}
+
+const char* const g = "foo"; // OK: internal linkage variable definition.
+static int h = 1; // OK: internal linkage variable definition.
+const int i = 1; // OK: internal linkage variable definition.
+extern int j; // OK: internal linkage variable definition.

clang-tools-extra/trunk/test/lit.cfg

 
 # testFormat: The test format to use to interpret tests.
 #
 # For now we require '&&' between commands, until they get globally killed and
 # the test runner updated.
 config.test_format = lit.formats.ShTest(execute_external)
 
 # suffixes: A list of file extensions to treat as test files.
-config.suffixes = ['.c', '.cpp', '.m', '.mm', '.cu', '.ll', '.cl', '.s', '.modularize', '.module-map-checker']
+config.suffixes = ['.c', '.cpp', '.hpp', '.m', '.mm', '.cu', '.ll', '.cl', '.s',
+  '.modularize', '.module-map-checker']
 
 # Test-time dependencies located in directories called 'Inputs' are excluded
 # from test suites; there won't be any lit tests within them.
 config.excludes = ['Inputs']
 
 # test_source_root: The root path where tests are located.
 config.test_source_root = os.path.dirname(__file__)