[clang-tidy] DefinitionsInHeadersCheck: Added option for checking C Code

[schrc3b6]

Summary

Eventhough there are no OCR rules in the C standard, to my
knowledge, it is recommended to not include definitions in header files.
This Patch introduces the CheckCCode Option which add C99 to the
language check. Since this checker was original written for C++ it
is off by default.

[schrc3b6]

fixed typo

[schrc3b6]

This is my first PR here, so if I missed something, I will be more than happy to change anything if you point me in the right direction.

I didn't add an extra test so far, because check_clang_tidy.py doesn't support the .h extension so far. And before adding C support to the test helper, I wanted to get feedback if this is something that would even get mainlined, since clang-tidy is mainly for c++.

[whisperity]

It would be interesting to add a test for this. I've recently came across the fact that Clang doesn't support common linkage definitions, namely that in C (but not in C++), if you do the following:

int I;
void f1(void) {}

int I;
void f2(void) {}

and compile these two source files and link them together, the two Is will refer in the resulting binary to the same symbol. (You must not put an initialiser here!) Will int I; being in a header, with no initialiser, be caught?

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

26

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

109

[schrc3b6]

In D111909#3069668, @whisperity wrote:

Will int I; being in a header, with no initialiser, be caught?

Currently it will be caught.

1 warning generated.
./foo.h:1:5: warning: variable 'i' defined in a header file; variable definitions in header files can lead to ODR violations [misc-definitions-in-headers]
int i;

I guess you don't want that to be cought if it is actually a tentative definition. If I remember correctly for clang and gcc -fno-common is the default.
I think we could do one of two things here either create no warning if there is a VarDelc without initialization or we could try to detect common linkage.
I will have a look if I can detect a change from inside the checker if the linkage of the variable changes via the fcommon compiler flag.

It would be interesting to add a test for this.

I will do so.

Thanks for the feedback.

[whisperity]

In D111909#3070593, @schrc3b6 wrote:

I guess you don't want that to be cought if it is actually a tentative definition. If I remember correctly for clang and gcc -fno-common is the default.
I think we could do one of two things here either create no warning if there is a VarDelc without initialization or we could try to detect common linkage.
I will have a look if I can detect a change from inside the checker if the linkage of the variable changes via the fcommon compiler flag.

My personal belief after having encountered common linkage is that it is disgusting and it's good that Clang doesn't support them (by default).

But I do not have any stake or practical experience with working long term on C programs to have a well-educated opinion on this.

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

 namespace misc {
 
 /// Finds non-extern non-inline function and variable definitions in header
 /// files, which can lead to potential ODR violations.
 ///
 /// The check supports these options:
 ///   - `UseHeaderFileExtension`: Whether to use file extension to distinguish
 ///     header files. True by default.
+///   - `CheckCCode`: Adds C99 to the minimum Language Requirements for this
+///     Checker. Disabled by default because this Checker wasn't build for C.
 ///   - `HeaderFileExtensions`: a semicolon-separated list of filename
 ///     extensions of header files (The filename extension should not contain
 ///     "." prefix). ";h;hh;hpp;hxx" by default.
 ///
 ///     For extension-less header files, using an empty string or leaving an
 ///     empty string between ";" if there are other filename extensions.
 ///
 /// 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);
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    if (CheckCCode) {
+      return LangOpts.CPlusPlus11 || LangOpts.C99;
+    }
     return LangOpts.CPlusPlus11;
   }
   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;
+  const bool CheckCCode;
   const std::string RawStringHeaderFileExtensions;
   utils::FileExtensionsSet HeaderFileExtensions;
 };
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_DEFINITIONS_IN_HEADERS_H

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

 }
 
 } // namespace
 
 DefinitionsInHeadersCheck::DefinitionsInHeadersCheck(StringRef Name,
                                                      ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       UseHeaderFileExtension(Options.get("UseHeaderFileExtension", true)),
+      CheckCCode(Options.get("CheckCCode", false)),
       RawStringHeaderFileExtensions(Options.getLocalOrGlobal(
           "HeaderFileExtensions", utils::defaultHeaderFileExtensions())) {
   if (!utils::parseFileExtensions(RawStringHeaderFileExtensions,
                                   HeaderFileExtensions,
                                   utils::defaultFileExtensionDelimiters())) {
     this->configurationDiag("Invalid header file extension: '%0'")
         << RawStringHeaderFileExtensions;
   }
 }
 
 void DefinitionsInHeadersCheck::storeOptions(
     ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "UseHeaderFileExtension", UseHeaderFileExtension);
+  Options.store(Opts, "CheckCCode", CheckCCode);
   Options.store(Opts, "HeaderFileExtensions", RawStringHeaderFileExtensions);
 }
 
 void DefinitionsInHeadersCheck::registerMatchers(MatchFinder *Finder) {
   auto DefinitionMatcher =
       anyOf(functionDecl(isDefinition(), unless(isDeleted())),
             varDecl(isDefinition()));
   if (UseHeaderFileExtension) {

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

.. option:: HeaderFileExtensions
    For header files without an extension, use an empty string (if there are no
    other desired extensions) or leave an empty element in the list. e.g.,
    "h,hh,hpp,hxx," (note the trailing comma).
 
 .. option:: UseHeaderFileExtension
 
    When `true`, the check will use the file extension to distinguish header
    files. Default is `true`.
+
+.. option:: CheckCCode
+
+   When `true` C99 is added to the minimum language requirements for this 
+   Cecker. Default is `false` because this Checker was not build for C.