[clang-tidy] Use ; as separator for HeaderFileExtensions

[jroelofs]

Summary

https://reviews.llvm.org/D74669#inline-685657

[jroelofs]

Fix comments

[lebedev.ri]

This will break existing .clang-tidy configs

[njames93]

The llvm::StringRef::split function can take multiple split characters, So it would be best to split on both for the time being but mark cases where , is used as deprecated. This way we keep the options in line with other clang-tidy checks but keep some backwards compatibility but in future change it to just using ;

[jroelofs]

Preserve backwards compatibility of ',' as a delimiter (for now).

The llvm::StringRef::split function can take multiple split characters,

AFAIU, that's for multi-character delimiters, not multiple delimiters.

[njames93]

In D75621#1905879, @jroelofs wrote:

Preserve backwards compatibility of ',' as a delimiter (for now).

The llvm::StringRef::split function can take multiple split characters,

AFAIU, that's for multi-character delimiters, not multiple delimiters.

Yeah just checked it out my bad.

clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp

40

if (AllFileExtensions.contains(Delimeter)) {

[aaron.ballman]

LGTM with a minor nit.

clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp

39–45

Can drop the top-level const here.

[lebedev.ri]

FWIW i'm personally still unconvinced this is fixing a real problem

clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp

42–44

Can this spam be avoided?
It's going to be really intrusive, and it is not always possible to
just perform the migration (think: using more than one clang-tidy version.)
The deprecation message should be in the docs/releasenotes.

[njames93]

clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp

42–44

That seems like a good idea. Would take a good many versions before the comma could safely be removed without fallout so people would get the warning too often and not be able to change it

[aaron.ballman]

clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp

42–44

FWIW, I was on the fence about this being a chatty diagnostic as well. I eventually figured that it was fine because it's going to stderr and it definitely gets the job done of letting people know. Most folks don't read release notes, so putting the information there is a good idea, but not really sufficient for warning about deprecation.

I don't think we have a better solution at hand and the point @lebedev.ri brings up about older versions is a valid one, so I agree, let's move this into the release notes rather than spamming the command line.

[jroelofs]

• Don't spam the deprecation message. Move that to release notes.
• Drop unnecessary const.

[jroelofs]

• Drop dead include.

[lebedev.ri]

No further comments from me, thanks

[njames93]

LGTM

clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h

44–46

Doesn't belong in this patch, but this function should be changed in a follow up to return an Optional<FileExtensionSet>.

[jroelofs]

clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h

44–46

Looking at the uses, I'm not convinced this would be better in this specific case.

Compare:

if (Optional<FileExtensionSet> HFE = parseFileExtensions(Extensions, Delimiters)) {
  HeaderFileExtensions = *HFE;
} else {
  errs() << "Invalid extensions: " << Extensions;
}

With the status quo:

if (!parseFileExtensions(Extensions, HeaderFileExtensions, Delimiters)) {
  errs() << "Invalid extensions: " << Extensions;
}

Optional's explicit operator bool is great for when you want to guard on the presence of the thing, but not so great when you want to check for it not being there. I feel like we'd need some new utility to go with Optional to make this nice:

if (not(HeaderFileExtensions) = parseFileExtensions(Extensions, Delimiters)) {
  errs() << "Invalid extensions: " << Extensions;
}

[njames93]

clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h

44–46

To be honest it should probably be Expected rather than Optional, but that still doesn't help solve the cleanliness issue.

[jroelofs]

clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h

44–46

Implementation of the not idea here:

https://gcc.godbolt.org/z/XZh39g

[jroelofs]

https://github.com/llvm/llvm-project/commit/47caa69120e582bf1b795ec646f069c83b0e9456

clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.h

 
 namespace clang {
 namespace tidy {
 namespace bugprone {
 
 /// Finds dynamically initialized static variables in header files.
 ///
 /// 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.
+///   - `HeaderFileExtensions`: a semicolon-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.
+///     empty string between ";" if there are other filename extensions.
 class DynamicStaticInitializersCheck : public ClangTidyCheck {
 public:
   DynamicStaticInitializersCheck(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:

clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp

 }
 
 DynamicStaticInitializersCheck::DynamicStaticInitializersCheck(StringRef Name,
                                                                ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       RawStringHeaderFileExtensions(Options.getLocalOrGlobal(
         "HeaderFileExtensions", utils::defaultHeaderFileExtensions())) {
   if (!utils::parseFileExtensions(RawStringHeaderFileExtensions,
-                                  HeaderFileExtensions, ',')) {
+                                  HeaderFileExtensions,
+                                  utils::defaultFileExtensionDelimiters())) {
     llvm::errs() << "Invalid header file extension: "
                  << RawStringHeaderFileExtensions << "\n";
   }
 }
 
 void DynamicStaticInitializersCheck::storeOptions(
     ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "HeaderFileExtensions", RawStringHeaderFileExtensions);

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

 namespace tidy {
 namespace google {
 namespace readability {
 
 /// 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.
+///   - `HeaderFileExtensions`: a semicolon-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.
+///     empty string between ";" if there are other filename extensions.
 class GlobalNamesInHeadersCheck : public ClangTidyCheck {
 public:
   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:

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

 namespace readability {
 
 GlobalNamesInHeadersCheck::GlobalNamesInHeadersCheck(StringRef Name,
                                                      ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       RawStringHeaderFileExtensions(Options.getLocalOrGlobal(
           "HeaderFileExtensions", utils::defaultHeaderFileExtensions())) {
   if (!utils::parseFileExtensions(RawStringHeaderFileExtensions,
-                                  HeaderFileExtensions, ',')) {
+                                  HeaderFileExtensions,
+                                  utils::defaultFileExtensionDelimiters())) {
     llvm::errs() << "Invalid header file extension: "
                  << RawStringHeaderFileExtensions << "\n";
   }
 }
 
 void GlobalNamesInHeadersCheck::storeOptions(
     ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "HeaderFileExtensions", RawStringHeaderFileExtensions);

clang-tools-extra/clang-tidy/google/UnnamedNamespaceInHeaderCheck.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.
+///   - `HeaderFileExtensions`: a semicolon-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.
+///     empty string between ";" if there are other filename extensions.
 ///
 /// https://google.github.io/styleguide/cppguide.html#Namespaces
 ///
 /// Corresponding cpplint.py check name: 'build/namespaces'.
 class UnnamedNamespaceInHeaderCheck : public ClangTidyCheck {
 public:
   UnnamedNamespaceInHeaderCheck(StringRef Name, ClangTidyContext *Context);
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;

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

 namespace build {
 
 UnnamedNamespaceInHeaderCheck::UnnamedNamespaceInHeaderCheck(
     StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       RawStringHeaderFileExtensions(Options.getLocalOrGlobal(
           "HeaderFileExtensions", utils::defaultHeaderFileExtensions())) {
   if (!utils::parseFileExtensions(RawStringHeaderFileExtensions,
-                                  HeaderFileExtensions, ',')) {
+                                  HeaderFileExtensions,
+                                  utils::defaultFileExtensionDelimiters())) {
     llvm::errs() << "Invalid header file extension: "
                  << RawStringHeaderFileExtensions << "\n";
   }
 }
 
 void UnnamedNamespaceInHeaderCheck::storeOptions(
     ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "HeaderFileExtensions", RawStringHeaderFileExtensions);

clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.h

 namespace clang {
 namespace tidy {
 namespace llvm_check {
 
 /// Finds and fixes header guards that do not adhere to LLVM style.
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/llvm-header-guard.html
 /// The check supports these options:
-///   - `HeaderFileExtensions`: a comma-separated list of filename extensions of
-///     header files (The filename extension should not contain "." prefix).
-///     ",h,hh,hpp,hxx" by default.
+///   - `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.
+///     empty string between ";" if there are other filename extensions.
 class LLVMHeaderGuardCheck : public utils::HeaderGuardCheck {
 public:
   LLVMHeaderGuardCheck(StringRef Name, ClangTidyContext *Context);
 
   bool shouldSuggestEndifComment(StringRef Filename) override { return false; }
   std::string getHeaderGuard(StringRef Filename, StringRef OldGuard) override;
 };
 
 } // namespace llvm_check
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_HEADERGUARDCHECK_H

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.
-///   - `HeaderFileExtensions`: a comma-separated list of filename extensions of
-///     header files (The filename extension should not contain "." prefix).
-///     ",h,hh,hpp,hxx" by default.
+///   - `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.
+///     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;

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

 
 DefinitionsInHeadersCheck::DefinitionsInHeadersCheck(StringRef Name,
                                                      ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       UseHeaderFileExtension(Options.get("UseHeaderFileExtension", true)),
       RawStringHeaderFileExtensions(Options.getLocalOrGlobal(
           "HeaderFileExtensions", utils::defaultHeaderFileExtensions())) {
   if (!utils::parseFileExtensions(RawStringHeaderFileExtensions,
-                                  HeaderFileExtensions, ',')) {
+                                  HeaderFileExtensions,
+                                  utils::defaultFileExtensionDelimiters())) {
     // FIXME: Find a more suitable way to handle invalid configuration
     // options.
     llvm::errs() << "Invalid header file extension: "
                  << RawStringHeaderFileExtensions << "\n";
   }
 }
 
 void DefinitionsInHeadersCheck::storeOptions(

clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h

bool isPresumedLocInHeaderFile(SourceLocation Loc, SourceManager &SM,
                                const FileExtensionsSet &HeaderFileExtensions);
 
 /// Checks whether spelling location of \p Loc is in header file.
 bool isSpellingLocInHeaderFile(SourceLocation Loc, SourceManager &SM,
                                const FileExtensionsSet &HeaderFileExtensions);
 
 /// Returns recommended default value for the list of header file
 /// extensions.
-inline StringRef defaultHeaderFileExtensions() { return ",h,hh,hpp,hxx"; }
+inline StringRef defaultHeaderFileExtensions() { return ";h;hh;hpp;hxx"; }
+
+/// Returns recommended default value for the list of file extension
+/// delimiters.
+inline StringRef defaultFileExtensionDelimiters() { return ",;"; }
 
 /// Parses header file extensions from a semicolon-separated list.
 bool parseFileExtensions(StringRef AllFileExtensions,
-                         FileExtensionsSet &FileExtensions, char Delimiter);
+                         FileExtensionsSet &FileExtensions,
+                         StringRef Delimiters);
 
 /// Decides whether a file has one of the specified file extensions.
 bool isFileExtension(StringRef FileName,
                      const FileExtensionsSet &FileExtensions);
 
 } // namespace utils
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_HEADER_FILE_EXTENSIONS_UTILS_H

clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp

 
 bool isSpellingLocInHeaderFile(SourceLocation Loc, SourceManager &SM,
                                const FileExtensionsSet &HeaderFileExtensions) {
   SourceLocation SpellingLoc = SM.getSpellingLoc(Loc);
   return isFileExtension(SM.getFilename(SpellingLoc), HeaderFileExtensions);
 }
 
 bool parseFileExtensions(StringRef AllFileExtensions,
-                         FileExtensionsSet &FileExtensions, char Delimiter) {
+                         FileExtensionsSet &FileExtensions,
+                         StringRef Delimiters) {
   SmallVector<StringRef, 5> Suffixes;
+  for (char Delimiter : Delimiters) {
+    if (AllFileExtensions.contains(Delimiter)) {
       AllFileExtensions.split(Suffixes, Delimiter);
+      break;
+    }
+  }
+
   FileExtensions.clear();
   for (StringRef Suffix : Suffixes) {
     StringRef Extension = Suffix.trim();
     if (!llvm::all_of(Extension, isAlphanumeric))
       return false;
     FileExtensions.insert(Extension);
   }
   return true;

clang-tools-extra/clang-tidy/utils/HeaderGuard.h

 #include "../utils/FileExtensionsUtils.h"
 
 namespace clang {
 namespace tidy {
 namespace utils {
 
 /// Finds and fixes header guards.
 /// The check supports these options:
-///   - `HeaderFileExtensions`: a comma-separated list of filename extensions of
-///     header files (The filename extension should not contain "." prefix).
-///     ",h,hh,hpp,hxx" by default.
+///   - `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.
+///     empty string between ";" if there are other filename extensions.
 class HeaderGuardCheck : public ClangTidyCheck {
 public:
   HeaderGuardCheck(StringRef Name, ClangTidyContext *Context)
       : ClangTidyCheck(Name, Context),
         RawStringHeaderFileExtensions(Options.getLocalOrGlobal(
             "HeaderFileExtensions", utils::defaultHeaderFileExtensions())) {
     utils::parseFileExtensions(RawStringHeaderFileExtensions,
-                               HeaderFileExtensions, ',');
+                               HeaderFileExtensions,
+                               utils::defaultFileExtensionDelimiters());
   }
   void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
                            Preprocessor *ModuleExpanderPP) override;
 
   /// Returns ``true`` if the check should suggest inserting a trailing comment
   /// on the ``#endif`` of the header guard. It will use the same name as
   /// returned by ``HeaderGuardCheck::getHeaderGuard``.
   virtual bool shouldSuggestEndifComment(StringRef Filename);

clang-tools-extra/docs/ReleaseNotes.rst

- Improved :doc:`readability-qualified-auto
   typed with ``auto *`` and ``auto &``.
 
 - Improved :doc:`readability-redundant-string-init
   <clang-tidy/checks/readability-redundant-string-init>` check now supports a
   `StringNames` option enabling its application to custom string classes. The
   check now detects in class initializers and constructor initializers which
   are deemed to be redundant.
 
+- Checks supporting the ``HeaderFileExtensions`` flag now support ``;`` as a
+  delimiter in addition to ``,``, with the latter being deprecated as of this
+  release. This simplifies how one specifies the options on the command line:
+  ``--config="{CheckOptions: [{ key: HeaderFileExtensions, value: h;;hpp;hxx }]}"``
+
 Renamed checks
 ^^^^^^^^^^^^^^
 
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...