[clang-tdiy] Add header file extension configuration support.

[hokein]

Summary

• Add a HeaderFileExtensions check option in misc-definitions-in-headers, google-build-namespaces and google-global-names-in-headers.

[aaron.ballman]

Public-facing documentation should also be updated to specify the new option for the various checkers.

[aaron.ballman]

clang-tidy/ClangTidy.h

55 ↗

(On Diff #44633)

"in both options" -> "in either list"

57 ↗

(On Diff #44633)

Any reason for Default to not be a StringRef (I assume this is always going to be a hard-coded string literal in callers)? At the very least, it should be a const std::string &.

clang-tidy/ClangTidyOptions.cpp

269 ↗

(On Diff #44633)

This code seems like it will fail if there are spaces in the extension list, like .h, .hxx instead of .h,.hxx. I think the list, as given by the end user, should be processed into a vector of sorts so that things can be normalized (entries without a period can be diagnosed, spaces removed, etc).

270 ↗

(On Diff #44633)

clang-format the patch.

clang-tidy/ClangTidyOptions.h

216 ↗

(On Diff #44633)

endsWithHeaderFileExtension instead? However, given that uses of this all start with a SourceLocation, I wonder if that makes for a cleaner API: isLocInHeaderFile(SourceLocation, <Extensions>);

Also, how does this work if I want to include an extension-less file as the header file "extension?" It would be plausible if the extensions were passed in as a list, but as it stands it doesn't seem possible without weird conventions like leaving a blank in the list (e.g., .h,,.hpp), which seems error-prone.

Also, I'm not certain what I can pass in. The documentation should be updated to state whether these extensions are intended to include the ".".

[hokein]

clang-tidy/ClangTidyOptions.h

216 ↗

(On Diff #44633)

endsWithHeaderFileExtension instead? However, given that uses of this all start with a SourceLocation, I wonder if that makes for a cleaner API: isLocInHeaderFile(SourceLocation, <Extensions>);

Using SourceLocation only is not enough to retrieve the belonging file name (we need SourceManager too).

Also, how does this work if I want to include an extension-less file as the header file "extension?" It would be plausible if the extensions were passed in as a list, but as it stands it doesn't seem possible without weird conventions like leaving a blank in the list (e.g., .h,,.hpp), which seems error-prone.

Yeah, for extensions-less header file, you can pass the string like .h,,.hpp, which is a bit of weird. Do you have a better idea here? Passing a string into header-file-extensions seems the most reasonable choice.

[alexfh]

Thank you for the patch! I have a few concerns though. See inline comments.

clang-tidy/ClangTidy.cpp

348 ↗

(On Diff #44633)

There's no need to declare a vector: for (const string &s : {"a", "b", "c"}) ....

Also, I don't think it makes sense to create a vector of two elements and run a loop over it instead of duplicating three lines of code.

clang-tidy/ClangTidy.h

54 ↗

(On Diff #44633)

We need to explicitly specify the order in which the options are considered: local, global, default.

clang-tidy/ClangTidyOptions.cpp

269 ↗

(On Diff #44633)

It's rather inefficient to split the string each time the function is called. If even we needed this function, we would better pass an ArrayRef<StringRef>. But even better, we can use llvm::StringSet and get rid of this function at all (we might need a helper function to parse a comma-separated list of extensions to a StringSet though).

clang-tidy/ClangTidyOptions.h

216 ↗

(On Diff #44633)

This function doesn't belong here. I'm also not sure we need this function at all. First, it's ineffective to split the string containing the list of extensions each time. Second, if we store a set of extensions, then we can just search for the actual file extension in this set.

clang-tidy/google/GlobalNamesInHeadersCheck.h

23 ↗

(On Diff #44633)

nit: s/There is one option:/The check supports these options:/

24 ↗

(On Diff #44633)

s/the file extensions that regard as header file/a comma-separated list of filename extensions of header files/

I don't think this list should include the period. It's better to just store extensions as returned by llvm::sys::path::extension(), so that it can be simply looked up in a set.

Also, it's common to indent lists, so please insert two spaces at the beginning of this line and the next one.

33 ↗

(On Diff #44633)

As mentioned earlier, this can be a set of strings (e.g. llvm::StringSet).

clang-tidy/misc/DefinitionsInHeadersCheck.cpp

22 ↗

(On Diff #44633)

s/useHeaderFileExtension/usesHeaderFileExtension/

clang-tidy/tool/ClangTidyMain.cpp

78 ↗

(On Diff #44633)

We don't need a command-line flag for this. The regular way to configure clang-tidy is .clang-tidy files. However, if needed, check options can be configured from the command line via the -config= option.

[alexfh]

clang-tidy/ClangTidyOptions.h

216 ↗

(On Diff #44633)

isLocInHeaderFile(SourceLocation, ...) is a nice idea, but we'd need to be more specific: either isExpansionLocInHeaderFile(SourceLoc, ...) or isSpellingLocInHeaderFile(SourceLoc, ...) (or both).

[hokein]

clang-tidy/tool/ClangTidyMain.cpp

78 ↗

(On Diff #44633)

From my understanding, it is a global option for header-file-extensions, right? If we remove this command-line flag, there is only local header-file-extensions option remained for particular checks.

[alexfh]

clang-tidy/tool/ClangTidyMain.cpp

78 ↗

(On Diff #44633)

Both local and global options reside in CheckOptions and can be configured in the same way using either a .clang-tidy file or the -config= command-line option. The only difference between local and global options is that the name of the latter is not prefixed with the "CheckName.". Makes sense?

[aaron.ballman]

clang-tidy/ClangTidyOptions.h

216 ↗

(On Diff #44633)

Yeah, for extensions-less header file, you can pass the string like .h,,.hpp, which is a bit of weird. Do you have a better idea here? Passing a string into header-file-extensions seems the most reasonable choice.

I thought those user configurations from the command line were in YAML or JSON format, those both have the notion of lists, don't they? I would imagine this would take a SmallVectorImpl<StringRef/std::string> for the list of extensions.

216 ↗

(On Diff #44633)

isLocInHeaderFile(SourceLocation, ...) is a nice idea, but we'd need to be more specific: either isExpansionLocInHeaderFile(SourceLoc, ...) or isSpellingLocInHeaderFile(SourceLoc, ...) (or both).

That's true, and I would think both are reasonable to add. I rather prefer that as an API instead of passing around file names as strings, personally.

clang-tidy/tool/ClangTidyMain.cpp

78 ↗

(On Diff #44633)

Both local and global options reside in CheckOptions and can be configured in the same way using either a .clang-tidy file or the -config= command-line option. The only difference between local and global options is that the name of the latter is not prefixed with the "CheckName.". Makes sense?

Ah, thank you for that explanation. I learned something new today. ;-)

[alexfh]

clang-tidy/ClangTidyOptions.h

216 ↗

(On Diff #44633)

User configurations are stored in YAML, however, CheckOptions is a map of strings to strings, so we can't use YAML lists to represent lists of extensions.
I personally see nothing wrong with ",.h,.hh,.hpp" for example, to represent the list of extensions (<empty> being the first one makes it somewhat stand out and provided there's a good documentation, this shouldn't be too confusing).

[aaron.ballman]

clang-tidy/ClangTidyOptions.h

216 ↗

(On Diff #44633)

I personally see nothing wrong with ",.h,.hh,.hpp" for example, to represent the list of extensions (<empty> being the first one makes it somewhat stand out and provided there's a good documentation, this shouldn't be too confusing).

I find it to be more clever than intuitive. If it were not user-facing, I would be less concerned. I don't think users should have to read documentation to figure out the *syntax* of how to pass options if we can at all avoid it. ;-)

Regardless, I would like to separate the two concepts -- there's the way we expose the option to the users, and there's our internal APIs that we call. I don't think the internal API has to take such an awkward thing directly just because the user-facing option has to be that way currently.

[alexfh]

clang-tidy/ClangTidyOptions.h

216 ↗

(On Diff #44633)

This aligns with what I wrote in my first comment on this:

This function doesn't belong here. I'm also not sure we need this function at all. First, it's ineffective to split the string containing the list of extensions each time. Second, if we store a set of extensions, then we can just search for the actual file extension in this set.

I still think the function doesn't belong here. And we also shouldn't split the string each time we call it. So I suggest adding these functions to clang-tidy/utils/:

bool isExpansionLocInHeader(const? SourceManager &SM, SourceLocation Loc, const StringSet &HeaderFileExtensions);
bool isSpellingLocInHeader(const? SourceManager &SM, SourceLocation Loc, const StringSet &HeaderFileExtensions);
StringSet splitCommaSeparatedSet(StringRef S); // Or something similar.

[hokein]

Address review comments.

[hokein]

clang-tidy/ClangTidy.h

65 ↗

(On Diff #44951)

Makes sense. I just refer from std::string get(StringRef LocalName, std::string Default) const. Also change get to use StringRef.

clang-tidy/tool/ClangTidyMain.cpp

78 ↗

(On Diff #44633)

I see it.

[hokein]

Add extension-less header file doc.

[aaron.ballman]

clang-tidy/utils/HeaderFileExtensionsUtils.cpp

48 ↗

(On Diff #45006)

One thing we may want to reconsider for this function is using comma as a delimiter. We have another in-flight review which has to use semi-colons for a checker option list because commas are part of the syntax for a type description (think Foo<A, B>). It would be really nice to have one utility for parsing lists of information from checker options, and for that to use the same delimiter everywhere so that users aren't confused by using commas in some places and semi-colons in others.

[hokein]

clang-tidy/ClangTidyOptions.cpp

269 ↗

(On Diff #44633)

Now it's tolerant of whitespace around the commas.

[hokein]

Format code style.

[aaron.ballman]

clang-tidy/google/GlobalNamesInHeadersCheck.h

26 ↗

(On Diff #45077)

s/includ/include

Is "." tolerated in the extension? It sounds like you can write: h,.hpp,hxx and it will work? Best to clarify the requirements (here and elsewhere). I don't have a strong opinion on whether the extension should include the full stop or not.

28 ↗

(On Diff #45077)

What if you only want to match extensionless header files?

clang-tidy/google/UnnamedNamespaceInHeaderCheck.h

25 ↗

(On Diff #45077)

s/includ/include

Same comment about use of ".".

clang-tidy/misc/DefinitionsInHeadersCheck.h

27 ↗

(On Diff #45077)

s/includ/include

Also, same comments about the full stop.

[hokein]

Update comments.

[hokein]

clang-tidy/google/GlobalNamesInHeadersCheck.h

26 ↗

(On Diff #45077)

The "." is not tolerated in the extension. Having updated comments.

28 ↗

(On Diff #45077)

As the comment states, using an empty string "" only match extension-less header files.

[aaron.ballman]

Mostly looks good, except I do still have the question about list delimiters that I'd like to hear some thoughts on. My primary concern is that I hope to avoid having different separators for different lists, and I wonder whether we want a common "parse a list of stuff" interface for the options that then gets used by functions like parseHeaderFileExtensions() to do sanitization of the list as-needed.

[hokein]

clang-tidy/utils/HeaderFileExtensionsUtils.cpp

49 ↗

(On Diff #45349)

I'm +1 on keeping the consistence of list option.

I also check the option in clang-tidy checks, seems that this is the first time to introduce list option into clang-tidy. So ; is fine to me.

[hokein]

Change delimeter from "," to ";".

[aaron.ballman]

LGTM, thank you for working on this!

[hokein]

ping @alexfh.

[alexfh]

Sorry for the delay. A few more comments.

clang-tidy/google/GlobalNamesInHeadersCheck.cpp

30 ↗

(On Diff #45852)

That's not the best way to report errors, especially, when clang-tidy is used as a library. There are multiple possibilities, e.g. add a callback to ClangTidyContext (or a method to ClangTidyCheck) to report a problem with configuration, or move to two-stage initialization of checks, or something similar. No need to change this now, but please add a FIXME to find a more suitable way to handle invalid configuration options.

59 ↗

(On Diff #45852)

No need for the parentheses around Result.SourceManager.

clang-tidy/utils/HeaderFileExtensionsUtils.cpp

50 ↗

(On Diff #45852)

seems that this is the first time to introduce list option into clang-tidy.

Comma is used as a delimiter in two more checks' options: clang-tidy/modernize/UseNullptrCheck.cpp and clang-tidy/misc/AssertSideEffectCheck.cpp. It's also used on the command line for the -checks and -warnings-as-errors options. I'm not sure what to do here, if we want consistency. For now, I'd rather switch back to comma and maybe provide a more generic facility to parse lists from clang-tidy options, which could also handle both separators in a smart way (e.g. when the list contains semicolons, use only them - this will allow listing entries like "A<B,T>", otherwise use comma; maybe also support some other separator, say | for cases when we need to list entries containing semicolons).

clang-tidy/utils/HeaderFileExtensionsUtils.h

23 ↗

(On Diff #45852)

That's an unnecessary long and too specific namespace name. I'd go for just utils, at least for now.

33 ↗

(On Diff #45852)

Don't add unused functions. When someone needs them, they can add them.

[hokein]

Address Alex's comments.

[hokein]

clang-tidy/utils/HeaderFileExtensionsUtils.cpp

51 ↗

(On Diff #46897)

Thanks for the explanation.
Now I add delimiter parameter to this function to allow developers to set specified character.

clang-tidy/utils/HeaderFileExtensionsUtils.h

34 ↗

(On Diff #46897)

It is used in UnnamedNamespaceInHeaderCheck.

[alexfh]

Looks good with a couple of comments. Thanks for working on this!

clang-tidy/utils/HeaderFileExtensionsUtils.cpp

45 ↗

(On Diff #46897)

No need for llvm:: here.

46 ↗

(On Diff #46897)

Actually, this could be changed to a more generic parseStringSetOption or something similar. Let's do this in a follow up.

But for now we need to change the output parameter type to llvm::SmallSetImpl<StringRef> to avoid hardcoding the small size template argument. The typedef is then not useful anymore.

clang-tidy/utils/HeaderFileExtensionsUtils.h

34 ↗

(On Diff #46897)

Ah, missed this somehow. Then it's fine. I'm not sure though, whether PresumedLocation is the right thing in that check, but we can figure this out later.

[alexfh]

clang-tidy/misc/DefinitionsInHeadersCheck.cpp

40 ↗

(On Diff #46897)

It's common to use just FIXME:, without the reference to the author.

[hokein]

clang-tidy/utils/HeaderFileExtensionsUtils.cpp

46 ↗

(On Diff #46897)

But for now we need to change the output parameter type to llvm::SmallSetImpl<StringRef> to avoid hardcoding the small size template argument. The typedef is then not useful anymore.

I can't find llvm::SmallSetImpl or any similar Set without size template argument.

[alexfh]

clang-tidy/utils/HeaderFileExtensionsUtils.cpp

46 ↗

(On Diff #46897)

Hm, indeed SmallSet has no base class unlike SmallVector, for example. Ok, then seems good for now.

[hokein]

Update.

[alexfh]

Still looks good.

[alexfh]

A couple of post-commit comments.

clang-tools-extra/trunk/clang-tidy/google/GlobalNamesInHeadersCheck.h

24

BTW, this belongs to the user-facing doc, so please move this to the .rst file. Same for other checks.

clang-tools-extra/trunk/clang-tidy/utils/HeaderFileExtensionsUtils.cpp

53

BTW, you should be able to use a range-based for loop.

clang-tools-extra/trunk/clang-tidy/ClangTidy.h

public:
   OptionsView(StringRef CheckName,
               const ClangTidyOptions::OptionMap &CheckOptions);
 
   /// \brief Read a named option from the \c Context.
   ///
   /// Reads the option with the check-local name \p LocalName from the
   /// \c CheckOptions. If the corresponding key is not present, returns
   /// \p Default.
-  std::string get(StringRef LocalName, std::string Default) const;
+  std::string get(StringRef LocalName, StringRef Default) const;
+
+  /// \brief Read a named option from the \c Context.
+  ///
+  /// Reads the option with the check-local name \p LocalName from local or
+  /// global \c CheckOptions. Gets local option first. If local is not
+  /// present, falls back to get global option. If global option is not present
+  /// either, returns Default.
+  std::string getLocalOrGlobal(StringRef LocalName, StringRef Default) const;
 
   /// \brief Read a named option from the \c Context and parse it as an integral
   /// type \c T.
   ///
   /// Reads the option with the check-local name \p LocalName from the
   /// \c CheckOptions. If the corresponding key is not present, returns
   /// \p Default.
   template <typename T>

clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp

void ClangTidyCheck::run(const ast_matchers::MatchFinder::MatchResult &Result) {
   Context->setSourceManager(Result.SourceManager);
   check(Result);
 }
 
 OptionsView::OptionsView(StringRef CheckName,
                          const ClangTidyOptions::OptionMap &CheckOptions)
     : NamePrefix(CheckName.str() + "."), CheckOptions(CheckOptions) {}
 
-std::string OptionsView::get(StringRef LocalName, std::string Default) const {
+std::string OptionsView::get(StringRef LocalName, StringRef Default) const {
   const auto &Iter = CheckOptions.find(NamePrefix + LocalName.str());
   if (Iter != CheckOptions.end())
     return Iter->second;
   return Default;
 }
 
+std::string OptionsView::getLocalOrGlobal(StringRef LocalName,
+                                          StringRef Default) const {
+  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;
+}
+
 void OptionsView::store(ClangTidyOptions::OptionMap &Options,
                         StringRef LocalName, StringRef Value) const {
   Options[NamePrefix + LocalName.str()] = Value;
 }
 
 void OptionsView::store(ClangTidyOptions::OptionMap &Options,
                         StringRef LocalName, int64_t Value) const {
   store(Options, LocalName, llvm::itostr(Value));

clang-tools-extra/trunk/clang-tidy/google/GlobalNamesInHeadersCheck.h

 //===--- GlobalNamesInHeadersCheck.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_GOOGLE_GLOBALNAMESINHEADERSCHECK_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_GLOBALNAMESINHEADERSCHECK_H
 
 #include "../ClangTidy.h"
+#include "../utils/HeaderFileExtensionsUtils.h"
 
 namespace clang {
 namespace tidy {
 namespace google {
 namespace readability {
 
-// Flag global namespace pollution in header files.
-// Right now it only triggers on using declarations and directives.
+/// Flag global namespace pollution in header files.
+/// Right now it only triggers on using declarations and directives.
+///
+/// The check supports these options:
+///   - `HeaderFileExtensions`: a comma-separated list of filename extensions
+///     of header files (The filename extensions should not contain "." prefix).
+///     "h" by default.
+///     For extension-less header files, using an empty string or leaving an
+///     empty string between "," if there are other filename extensions.
 class GlobalNamesInHeadersCheck : public ClangTidyCheck {
 public:
-  GlobalNamesInHeadersCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+  GlobalNamesInHeadersCheck(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 std::string RawStringHeaderFileExtensions;
+  utils::HeaderFileExtensionsSet HeaderFileExtensions;
 };
 
 } // namespace readability
 } // namespace google
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_GLOBALNAMESINHEADERSCHECK_H

clang-tools-extra/trunk/clang-tidy/google/GlobalNamesInHeadersCheck.cpp

 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace google {
 namespace readability {
 
+GlobalNamesInHeadersCheck::GlobalNamesInHeadersCheck(StringRef Name,
+                                                     ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      RawStringHeaderFileExtensions(
+          Options.getLocalOrGlobal("HeaderFileExtensions", "h")) {
+  if (!utils::parseHeaderFileExtensions(RawStringHeaderFileExtensions,
+                                        HeaderFileExtensions,
+                                        ',')) {
+    llvm::errs() << "Invalid header file extension: "
+                 << RawStringHeaderFileExtensions << "\n";
+  }
+}
+
+void GlobalNamesInHeadersCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "HeaderFileExtensions", RawStringHeaderFileExtensions);
+}
+
 void
 GlobalNamesInHeadersCheck::registerMatchers(ast_matchers::MatchFinder *Finder) {
   Finder->addMatcher(
       decl(anyOf(usingDecl(), usingDirectiveDecl()),
            hasDeclContext(translationUnitDecl())).bind("using_decl"),
       this);
 }
 
 void GlobalNamesInHeadersCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *D = Result.Nodes.getNodeAs<Decl>("using_decl");
   // If it comes from a macro, we'll assume it is fine.
   if (D->getLocStart().isMacroID())
     return;
 
   // Ignore if it comes from the "main" file ...
   if (Result.SourceManager->isInMainFile(
           Result.SourceManager->getExpansionLoc(D->getLocStart()))) {
     // unless that file is a header.
-    StringRef Filename = Result.SourceManager->getFilename(
-        Result.SourceManager->getSpellingLoc(D->getLocStart()));
-
-    if (!Filename.endswith(".h"))
+    if (!utils::isSpellingLocInHeaderFile(
+            D->getLocStart(), *Result.SourceManager, HeaderFileExtensions))
       return;
   }
 
   if (const auto* UsingDirective = dyn_cast<UsingDirectiveDecl>(D)) {
     if (UsingDirective->getNominatedNamespace()->isAnonymousNamespace()) {
       // Anynoumous namespaces inject a using directive into the AST to import
       // the names into the containing namespace.
       // We should not have them in headers, but there is another warning for

clang-tools-extra/trunk/clang-tidy/google/UnnamedNamespaceInHeaderCheck.h

 //===--- UnnamedNamespaceInHeaderCheck.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_GOOGLE_UNNAMEDNAMESPACEINHEADERCHECK_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_UNNAMEDNAMESPACEINHEADERCHECK_H
 
 #include "../ClangTidy.h"
+#include "../utils/HeaderFileExtensionsUtils.h"
 
 namespace clang {
 namespace tidy {
 namespace google {
 namespace build {
 
 /// Finds anonymous namespaces in headers.
 ///
+/// The check supports these options:
+///   - `HeaderFileExtensions`: a comma-separated list of filename extensions of
+///     header files (The filename extensions 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.
+///
 /// http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namespaces#Namespaces
 ///
 /// Corresponding cpplint.py check name: 'build/namespaces'.
 class UnnamedNamespaceInHeaderCheck : public ClangTidyCheck {
 public:
-  UnnamedNamespaceInHeaderCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+  UnnamedNamespaceInHeaderCheck(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 std::string RawStringHeaderFileExtensions;
+  utils::HeaderFileExtensionsSet HeaderFileExtensions;
 };
 
 } // namespace build
 } // namespace google
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_UNNAMEDNAMESPACEINHEADERCHECK_H

clang-tools-extra/trunk/clang-tidy/google/UnnamedNamespaceInHeaderCheck.cpp

 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace google {
 namespace build {
 
+UnnamedNamespaceInHeaderCheck::UnnamedNamespaceInHeaderCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      RawStringHeaderFileExtensions(
+          Options.getLocalOrGlobal("HeaderFileExtensions", "h,hh,hpp,hxx")) {
+  if (!utils::parseHeaderFileExtensions(RawStringHeaderFileExtensions,
+                                        HeaderFileExtensions,
+                                        ',')) {
+    llvm::errs() << "Invalid header file extension: "
+                 << RawStringHeaderFileExtensions << "\n";
+  }
+}
+
+void UnnamedNamespaceInHeaderCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "HeaderFileExtensions", RawStringHeaderFileExtensions);
+}
+
 void UnnamedNamespaceInHeaderCheck::registerMatchers(
     ast_matchers::MatchFinder *Finder) {
   // Only register the matchers for C++; the functionality currently does not
   // provide any benefit to other languages, despite being benign.
   if (getLangOpts().CPlusPlus)
     Finder->addMatcher(namespaceDecl(isAnonymous()).bind("anonymousNamespace"),
                        this);
 }
 
 void
 UnnamedNamespaceInHeaderCheck::check(const MatchFinder::MatchResult &Result) {
-  SourceManager *SM = Result.SourceManager;
   const auto *N = Result.Nodes.getNodeAs<NamespaceDecl>("anonymousNamespace");
   SourceLocation Loc = N->getLocStart();
   if (!Loc.isValid())
     return;
 
-  // Look if we're inside a header, check for common suffixes only.
-  // TODO: Allow configuring the set of file extensions.
-  StringRef FileName = SM->getPresumedLoc(Loc).getFilename();
-  if (FileName.endswith(".h") || FileName.endswith(".hh") ||
-      FileName.endswith(".hpp") || FileName.endswith(".hxx"))
+  if (utils::isPresumedLocInHeaderFile(Loc, *Result.SourceManager,
+                                       HeaderFileExtensions))
     diag(Loc, "do not use unnamed namespaces in header files");
 }
 
 } // namespace build
 } // namespace google
 } // namespace tidy
 } // namespace clang

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"
+#include "../utils/HeaderFileExtensionsUtils.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
+/// 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.
+///   - `HeaderFileExtensions`: a comma-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);
   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 std::string RawStringHeaderFileExtensions;
+  utils::HeaderFileExtensionsSet HeaderFileExtensions;
 };
 
 } // 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

 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();
+AST_MATCHER_P(NamedDecl, usesHeaderFileExtension,
+              utils::HeaderFileExtensionsSet, HeaderFileExtensions) {
+  return utils::isExpansionLocInHeaderFile(
+      Node.getLocStart(), Finder->getASTContext().getSourceManager(),
+      HeaderFileExtensions);
 }
 
 } // namespace
 
-DefinitionsInHeadersCheck::DefinitionsInHeadersCheck(
-    StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context),
-        UseHeaderFileExtension(Options.get("UseHeaderFileExtension", true)) {}
+DefinitionsInHeadersCheck::DefinitionsInHeadersCheck(StringRef Name,
+                                                     ClangTidyContext *Context)
+      UseHeaderFileExtension(Options.get("UseHeaderFileExtension", true)),
+      RawStringHeaderFileExtensions(
+          Options.getLocalOrGlobal("HeaderFileExtensions", ",h,hh,hpp,hxx")) {
+  if (!utils::parseHeaderFileExtensions(RawStringHeaderFileExtensions,
+                                        HeaderFileExtensions,
+                                        ',')) {
+    // FIXME: Find a more suitable way to handle invalid configuration
+    // options.
+    llvm::errs() << "Invalid header file extension: "
+                 << RawStringHeaderFileExtensions << "\n";
+  }
+}
 
 void DefinitionsInHeadersCheck::storeOptions(
     ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "UseHeaderFileExtension", UseHeaderFileExtension);
+  Options.store(Opts, "HeaderFileExtensions", RawStringHeaderFileExtensions);
 }
 
 void DefinitionsInHeadersCheck::registerMatchers(MatchFinder *Finder) {
   if (UseHeaderFileExtension) {
     Finder->addMatcher(
         namedDecl(anyOf(functionDecl(isDefinition()), varDecl(isDefinition())),
-                  isHeaderFileExtension()).bind("name-decl"),
+                  usesHeaderFileExtension(HeaderFileExtensions))
+            .bind("name-decl"),
         this);
   } else {
     Finder->addMatcher(
         namedDecl(anyOf(functionDecl(isDefinition()), varDecl(isDefinition())),
-                  anyOf(isHeaderFileExtension(),
-                        unless(isExpansionInMainFile()))).bind("name-decl"),
+                  anyOf(usesHeaderFileExtension(HeaderFileExtensions),
+                        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

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

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyUtils
   HeaderGuard.cpp
+  HeaderFileExtensionsUtils.cpp
   IncludeInserter.cpp
   IncludeSorter.cpp
   LexerUtils.cpp
   TypeTraits.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   )

clang-tools-extra/trunk/clang-tidy/utils/HeaderFileExtensionsUtils.h

+//===--- HeaderFileExtensionsUtils.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_UTILS_HEADER_FILE_EXTENSIONS_UTILS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_HEADER_FILE_EXTENSIONS_UTILS_H
+
+#include <string>
+
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/SmallSet.h"
+#include "llvm/Support/Path.h"
+
+namespace clang {
+namespace tidy {
+namespace utils {
+
+typedef llvm::SmallSet<llvm::StringRef, 5> HeaderFileExtensionsSet;
+
+/// \brief Checks whether expansion location of Loc is in header file.
+bool isExpansionLocInHeaderFile(
+    SourceLocation Loc, const SourceManager &SM,
+    const HeaderFileExtensionsSet &HeaderFileExtensions);
+
+/// \brief Checks whether presumed location of Loc is in header file.
+bool isPresumedLocInHeaderFile(
+    SourceLocation Loc, SourceManager &SM,
+    const HeaderFileExtensionsSet &HeaderFileExtensions);
+
+/// \brief Checks whether spelling location of Loc is in header file.
+bool isSpellingLocInHeaderFile(
+    SourceLocation Loc, SourceManager &SM,
+    const HeaderFileExtensionsSet &HeaderFileExtensions);
+
+/// \brief Parses header file extensions from a semicolon-separated list.
+bool parseHeaderFileExtensions(StringRef AllHeaderFileExtensions,
+                               HeaderFileExtensionsSet &HeaderFileExtensions,
+                               char delimiter);
+
+} // namespace utils
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_HEADER_FILE_EXTENSIONS_UTILS_H

clang-tools-extra/trunk/clang-tidy/utils/HeaderFileExtensionsUtils.cpp

+//===--- HeaderFileExtensionsUtils.cpp - clang-tidy--------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "HeaderFileExtensionsUtils.h"
+#include "clang/Basic/CharInfo.h"
+
+namespace clang {
+namespace tidy {
+namespace utils {
+
+bool isExpansionLocInHeaderFile(
+    SourceLocation Loc, const SourceManager &SM,
+    const HeaderFileExtensionsSet &HeaderFileExtensions) {
+  SourceLocation ExpansionLoc = SM.getExpansionLoc(Loc);
+  StringRef FileExtension =
+      llvm::sys::path::extension(SM.getFilename(ExpansionLoc));
+  return HeaderFileExtensions.count(FileExtension.substr(1)) > 0;
+}
+
+bool isPresumedLocInHeaderFile(
+    SourceLocation Loc, SourceManager &SM,
+    const HeaderFileExtensionsSet &HeaderFileExtensions) {
+  PresumedLoc PresumedLocation = SM.getPresumedLoc(Loc);
+  StringRef FileExtension =
+      llvm::sys::path::extension(PresumedLocation.getFilename());
+  return HeaderFileExtensions.count(FileExtension.substr(1)) > 0;
+}
+
+bool isSpellingLocInHeaderFile(
+    SourceLocation Loc, SourceManager &SM,
+    const HeaderFileExtensionsSet &HeaderFileExtensions) {
+  SourceLocation SpellingLoc = SM.getSpellingLoc(Loc);
+  StringRef FileExtension =
+      llvm::sys::path::extension(SM.getFilename(SpellingLoc));
+
+  return HeaderFileExtensions.count(FileExtension.substr(1)) > 0;
+}
+
+bool parseHeaderFileExtensions(StringRef AllHeaderFileExtensions,
+                               HeaderFileExtensionsSet &HeaderFileExtensions,
+                               char delimiter) {
+  SmallVector<StringRef, 5> Suffixes;
+  AllHeaderFileExtensions.split(Suffixes, delimiter);
+  HeaderFileExtensions.clear();
+  for (StringRef Suffix : Suffixes) {
+    StringRef Extension = Suffix.trim();
+    for (StringRef::const_iterator it = Extension.begin();
+         it != Extension.end(); ++it) {
+      if (!isAlphanumeric(*it))
+        return false;
+    }
+    HeaderFileExtensions.insert(Extension);
+  }
+  return true;
+}
+
+} // namespace utils
+} // namespace tidy
+} // namespace clang