[PATCH] D75621: [clang-tidy] Use ; as separator for HeaderFileExtensions

[Jonathan Roelofs via Phabricator via cfe-commits]

jroelofs closed this revision.
jroelofs added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75621/new/

https://reviews.llvm.org/D75621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75621: [clang-tidy] Use ; as separator for HeaderFileExtensions

[Jonathan Roelofs via Phabricator via cfe-commits]

jroelofs marked an inline comment as done.
jroelofs added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h:44-46
 bool parseFileExtensions(StringRef AllFileExtensions,
- FileExtensionsSet , char Delimiter);
+ FileExtensionsSet ,
+ StringRef Delimiters);

njames93 wrote:
> jroelofs wrote:
> > njames93 wrote:
> > > Doesn't belong in this patch, but this function should be changed in a 
> > > follow up to return an `Optional`.
> > Looking at the uses, I'm not convinced this would be better in this 
> > specific case.
> > 
> > Compare:
> > ```
> > if (Optional 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;
> > }
> > ```
> To be honest it should probably be `Expected` rather than `Optional`, but 
> that still doesn't help solve the cleanliness issue.
Implementation of the `not` idea here:

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75621/new/

https://reviews.llvm.org/D75621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75621: [clang-tidy] Use ; as separator for HeaderFileExtensions

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h:44-46
 bool parseFileExtensions(StringRef AllFileExtensions,
- FileExtensionsSet , char Delimiter);
+ FileExtensionsSet ,
+ StringRef Delimiters);

jroelofs wrote:
> njames93 wrote:
> > Doesn't belong in this patch, but this function should be changed in a 
> > follow up to return an `Optional`.
> Looking at the uses, I'm not convinced this would be better in this specific 
> case.
> 
> Compare:
> ```
> if (Optional 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;
> }
> ```
To be honest it should probably be `Expected` rather than `Optional`, but that 
still doesn't help solve the cleanliness issue.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75621/new/

https://reviews.llvm.org/D75621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75621: [clang-tidy] Use ; as separator for HeaderFileExtensions

[Jonathan Roelofs via Phabricator via cfe-commits]

jroelofs marked an inline comment as done.
jroelofs added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h:44-46
 bool parseFileExtensions(StringRef AllFileExtensions,
- FileExtensionsSet , char Delimiter);
+ FileExtensionsSet ,
+ StringRef Delimiters);

njames93 wrote:
> Doesn't belong in this patch, but this function should be changed in a follow 
> up to return an `Optional`.
Looking at the uses, I'm not convinced this would be better in this specific 
case.

Compare:
```
if (Optional 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;
}
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75621/new/

https://reviews.llvm.org/D75621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75621: [clang-tidy] Use ; as separator for HeaderFileExtensions

[Nathan James via Phabricator via cfe-commits]

njames93 accepted this revision.
njames93 added a comment.

LGTM




Comment at: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h:44-46
 bool parseFileExtensions(StringRef AllFileExtensions,
- FileExtensionsSet , char Delimiter);
+ FileExtensionsSet ,
+ StringRef Delimiters);

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75621/new/

https://reviews.llvm.org/D75621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75621: [clang-tidy] Use ; as separator for HeaderFileExtensions

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

No further comments from me, thanks


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75621/new/

https://reviews.llvm.org/D75621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75621: [clang-tidy] Use ; as separator for HeaderFileExtensions

[Jonathan Roelofs via Phabricator via cfe-commits]

jroelofs updated this revision to Diff 248757.
jroelofs added a comment.

- Drop dead include.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75621/new/

https://reviews.llvm.org/D75621

Files:
  clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.h
  clang-tools-extra/clang-tidy/google/GlobalNamesInHeadersCheck.cpp
  clang-tools-extra/clang-tidy/google/GlobalNamesInHeadersCheck.h
  clang-tools-extra/clang-tidy/google/UnnamedNamespaceInHeaderCheck.cpp
  clang-tools-extra/clang-tidy/google/UnnamedNamespaceInHeaderCheck.h
  clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.h
  clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.cpp
  clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.h
  clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
  clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
  clang-tools-extra/clang-tidy/utils/HeaderGuard.h
  clang-tools-extra/docs/ReleaseNotes.rst

Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -125,6 +125,11 @@
   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
 ^^
 
Index: clang-tools-extra/clang-tidy/utils/HeaderGuard.h
===
--- clang-tools-extra/clang-tidy/utils/HeaderGuard.h
+++ clang-tools-extra/clang-tidy/utils/HeaderGuard.h
@@ -18,11 +18,12 @@
 
 /// 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)
@@ -30,7 +31,8 @@
 RawStringHeaderFileExtensions(Options.getLocalOrGlobal(
 "HeaderFileExtensions", utils::defaultHeaderFileExtensions())) {
 utils::parseFileExtensions(RawStringHeaderFileExtensions,
-   HeaderFileExtensions, ',');
+   HeaderFileExtensions,
+   utils::defaultFileExtensionDelimiters());
   }
   void registerPPCallbacks(const SourceManager , Preprocessor *PP,
Preprocessor *ModuleExpanderPP) override;
Index: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
===
--- clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
+++ clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
@@ -34,11 +34,16 @@
 
 /// 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 , char Delimiter);
+ FileExtensionsSet ,
+ StringRef Delimiters);
 
 /// Decides whether a file has one of the specified file extensions.
 bool isFileExtension(StringRef FileName,
Index: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
@@ -33,9 +33,16 @@
 }
 
 bool parseFileExtensions(StringRef AllFileExtensions,
- FileExtensionsSet , char Delimiter) {
+ FileExtensionsSet ,
+ StringRef Delimiters) {
   SmallVector Suffixes;
-  

[PATCH] D75621: [clang-tidy] Use ; as separator for HeaderFileExtensions

[Jonathan Roelofs via Phabricator via cfe-commits]

jroelofs updated this revision to Diff 248755.
jroelofs added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75621/new/

https://reviews.llvm.org/D75621

Files:
  clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.h
  clang-tools-extra/clang-tidy/google/GlobalNamesInHeadersCheck.cpp
  clang-tools-extra/clang-tidy/google/GlobalNamesInHeadersCheck.h
  clang-tools-extra/clang-tidy/google/UnnamedNamespaceInHeaderCheck.cpp
  clang-tools-extra/clang-tidy/google/UnnamedNamespaceInHeaderCheck.h
  clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.h
  clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.cpp
  clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.h
  clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
  clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
  clang-tools-extra/clang-tidy/utils/HeaderGuard.h
  clang-tools-extra/docs/ReleaseNotes.rst

Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -125,6 +125,11 @@
   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
 ^^
 
Index: clang-tools-extra/clang-tidy/utils/HeaderGuard.h
===
--- clang-tools-extra/clang-tidy/utils/HeaderGuard.h
+++ clang-tools-extra/clang-tidy/utils/HeaderGuard.h
@@ -18,11 +18,12 @@
 
 /// 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)
@@ -30,7 +31,8 @@
 RawStringHeaderFileExtensions(Options.getLocalOrGlobal(
 "HeaderFileExtensions", utils::defaultHeaderFileExtensions())) {
 utils::parseFileExtensions(RawStringHeaderFileExtensions,
-   HeaderFileExtensions, ',');
+   HeaderFileExtensions,
+   utils::defaultFileExtensionDelimiters());
   }
   void registerPPCallbacks(const SourceManager , Preprocessor *PP,
Preprocessor *ModuleExpanderPP) override;
Index: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
===
--- clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
+++ clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
@@ -34,11 +34,16 @@
 
 /// 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 , char Delimiter);
+ FileExtensionsSet ,
+ StringRef Delimiters);
 
 /// Decides whether a file has one of the specified file extensions.
 bool isFileExtension(StringRef FileName,
Index: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
@@ -9,6 +9,7 @@
 #include "FileExtensionsUtils.h"
 #include "clang/Basic/CharInfo.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/raw_ostream.h"
 
 namespace clang {
 namespace tidy {
@@ -33,9 

[PATCH] D75621: [clang-tidy] Use ; as separator for HeaderFileExtensions

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp:43-45
+llvm::errs()
+<< "Using ',' as a file extension delimiter is deprecated. Please "
+   "switch your configuration to use ';'.\n";

njames93 wrote:
> lebedev.ri wrote:
> > 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.
> 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
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75621/new/

https://reviews.llvm.org/D75621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75621: [clang-tidy] Use ; as separator for HeaderFileExtensions

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp:43-45
+llvm::errs()
+<< "Using ',' as a file extension delimiter is deprecated. Please "
+   "switch your configuration to use ';'.\n";

lebedev.ri wrote:
> 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.
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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75621/new/

https://reviews.llvm.org/D75621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75621: [clang-tidy] Use ; as separator for HeaderFileExtensions

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

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




Comment at: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp:43-45
+llvm::errs()
+<< "Using ',' as a file extension delimiter is deprecated. Please "
+   "switch your configuration to use ';'.\n";

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75621/new/

https://reviews.llvm.org/D75621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75621: [clang-tidy] Use ; as separator for HeaderFileExtensions

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM with a minor nit.




Comment at: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp:40
   SmallVector Suffixes;
-  AllFileExtensions.split(Suffixes, Delimiter);
+  for (const char Delimiter : Delimiters) {
+if (AllFileExtensions.contains(Delimiter)) {

Can drop the top-level `const` here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75621/new/

https://reviews.llvm.org/D75621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75621: [clang-tidy] Use ; as separator for HeaderFileExtensions

[Jonathan Roelofs via Phabricator via cfe-commits]

jroelofs updated this revision to Diff 248357.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75621/new/

https://reviews.llvm.org/D75621

Files:
  clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.h
  clang-tools-extra/clang-tidy/google/GlobalNamesInHeadersCheck.cpp
  clang-tools-extra/clang-tidy/google/GlobalNamesInHeadersCheck.h
  clang-tools-extra/clang-tidy/google/UnnamedNamespaceInHeaderCheck.cpp
  clang-tools-extra/clang-tidy/google/UnnamedNamespaceInHeaderCheck.h
  clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.h
  clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.cpp
  clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.h
  clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
  clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
  clang-tools-extra/clang-tidy/utils/HeaderGuard.h

Index: clang-tools-extra/clang-tidy/utils/HeaderGuard.h
===
--- clang-tools-extra/clang-tidy/utils/HeaderGuard.h
+++ clang-tools-extra/clang-tidy/utils/HeaderGuard.h
@@ -18,11 +18,12 @@
 
 /// 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)
@@ -30,7 +31,8 @@
 RawStringHeaderFileExtensions(Options.getLocalOrGlobal(
 "HeaderFileExtensions", utils::defaultHeaderFileExtensions())) {
 utils::parseFileExtensions(RawStringHeaderFileExtensions,
-   HeaderFileExtensions, ',');
+   HeaderFileExtensions,
+   utils::defaultFileExtensionDelimiters());
   }
   void registerPPCallbacks(const SourceManager , Preprocessor *PP,
Preprocessor *ModuleExpanderPP) override;
Index: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
===
--- clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
+++ clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
@@ -34,11 +34,16 @@
 
 /// 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 , char Delimiter);
+ FileExtensionsSet ,
+ StringRef Delimiters);
 
 /// Decides whether a file has one of the specified file extensions.
 bool isFileExtension(StringRef FileName,
Index: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
@@ -9,6 +9,7 @@
 #include "FileExtensionsUtils.h"
 #include "clang/Basic/CharInfo.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/raw_ostream.h"
 
 namespace clang {
 namespace tidy {
@@ -33,9 +34,21 @@
 }
 
 bool parseFileExtensions(StringRef AllFileExtensions,
- FileExtensionsSet , char Delimiter) {
+ FileExtensionsSet ,
+ StringRef Delimiters) {
   SmallVector Suffixes;
-  AllFileExtensions.split(Suffixes, Delimiter);
+  for (const char Delimiter : Delimiters) {
+if (AllFileExtensions.contains(Delimiter)) {
+  if (Delimiter == ',') {
+llvm::errs()
+<< "Using ',' as a file extension delimiter is deprecated. Please "
+   "switch your configuration to use ';'.\n";
+  }
+  AllFileExtensions.split(Suffixes, Delimiter);
+  break;
+}
+  }
+
   FileExtensions.clear();
   for (StringRef Suffix : Suffixes) {
 StringRef Extension = Suffix.trim();
Index: clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.h

[PATCH] D75621: [clang-tidy] Use ; as separator for HeaderFileExtensions

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

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.




Comment at: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp:41
+  for (const char Delimiter : Delimiters) {
+if (StringRef::npos != AllFileExtensions.find(Delimiter)) {
+  if (Delimiter == ',') {

`if (AllFileExtensions.contains(Delimeter)) {`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75621/new/

https://reviews.llvm.org/D75621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75621: [clang-tidy] Use ; as separator for HeaderFileExtensions

[Jonathan Roelofs via Phabricator via cfe-commits]

jroelofs updated this revision to Diff 248245.
jroelofs added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75621/new/

https://reviews.llvm.org/D75621

Files:
  clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.h
  clang-tools-extra/clang-tidy/google/GlobalNamesInHeadersCheck.cpp
  clang-tools-extra/clang-tidy/google/GlobalNamesInHeadersCheck.h
  clang-tools-extra/clang-tidy/google/UnnamedNamespaceInHeaderCheck.cpp
  clang-tools-extra/clang-tidy/google/UnnamedNamespaceInHeaderCheck.h
  clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.h
  clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.cpp
  clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.h
  clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
  clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
  clang-tools-extra/clang-tidy/utils/HeaderGuard.h

Index: clang-tools-extra/clang-tidy/utils/HeaderGuard.h
===
--- clang-tools-extra/clang-tidy/utils/HeaderGuard.h
+++ clang-tools-extra/clang-tidy/utils/HeaderGuard.h
@@ -18,11 +18,12 @@
 
 /// 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)
@@ -30,7 +31,8 @@
 RawStringHeaderFileExtensions(Options.getLocalOrGlobal(
 "HeaderFileExtensions", utils::defaultHeaderFileExtensions())) {
 utils::parseFileExtensions(RawStringHeaderFileExtensions,
-   HeaderFileExtensions, ',');
+   HeaderFileExtensions,
+   utils::defaultFileExtensionDelimiters());
   }
   void registerPPCallbacks(const SourceManager , Preprocessor *PP,
Preprocessor *ModuleExpanderPP) override;
Index: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
===
--- clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
+++ clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
@@ -34,11 +34,16 @@
 
 /// 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 , char Delimiter);
+ FileExtensionsSet ,
+ StringRef Delimiters);
 
 /// Decides whether a file has one of the specified file extensions.
 bool isFileExtension(StringRef FileName,
Index: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
@@ -9,6 +9,7 @@
 #include "FileExtensionsUtils.h"
 #include "clang/Basic/CharInfo.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/raw_ostream.h"
 
 namespace clang {
 namespace tidy {
@@ -33,9 +34,21 @@
 }
 
 bool parseFileExtensions(StringRef AllFileExtensions,
- FileExtensionsSet , char Delimiter) {
+ FileExtensionsSet ,
+ StringRef Delimiters) {
   SmallVector Suffixes;
-  AllFileExtensions.split(Suffixes, Delimiter);
+  for (const char Delimiter : Delimiters) {
+if (StringRef::npos != AllFileExtensions.find(Delimiter)) {
+  if (Delimiter == ',') {
+llvm::errs()
+<< "Using ',' as a file extension delimiter is deprecated. Please "
+   "switch your configuration to use ';'.\n";
+  }
+

[PATCH] D75621: [clang-tidy] Use ; as separator for HeaderFileExtensions

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

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 `;`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75621/new/

https://reviews.llvm.org/D75621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75621: [clang-tidy] Use ; as separator for HeaderFileExtensions

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri requested changes to this revision.
lebedev.ri added a comment.
This revision now requires changes to proceed.

This will break existing `.clang-tidy` configs


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75621/new/

https://reviews.llvm.org/D75621



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75621: [clang-tidy] Use ; as separator for HeaderFileExtensions

[Jonathan Roelofs via Phabricator via cfe-commits]

jroelofs updated this revision to Diff 248206.
jroelofs added a comment.

Fix comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75621/new/

https://reviews.llvm.org/D75621

Files:
  clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.h
  clang-tools-extra/clang-tidy/google/GlobalNamesInHeadersCheck.cpp
  clang-tools-extra/clang-tidy/google/GlobalNamesInHeadersCheck.h
  clang-tools-extra/clang-tidy/google/UnnamedNamespaceInHeaderCheck.cpp
  clang-tools-extra/clang-tidy/google/UnnamedNamespaceInHeaderCheck.h
  clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.h
  clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.cpp
  clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.h
  clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
  clang-tools-extra/clang-tidy/utils/HeaderGuard.h

Index: clang-tools-extra/clang-tidy/utils/HeaderGuard.h
===
--- clang-tools-extra/clang-tidy/utils/HeaderGuard.h
+++ clang-tools-extra/clang-tidy/utils/HeaderGuard.h
@@ -18,11 +18,12 @@
 
 /// 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)
@@ -30,7 +31,7 @@
 RawStringHeaderFileExtensions(Options.getLocalOrGlobal(
 "HeaderFileExtensions", utils::defaultHeaderFileExtensions())) {
 utils::parseFileExtensions(RawStringHeaderFileExtensions,
-   HeaderFileExtensions, ',');
+   HeaderFileExtensions, ';');
   }
   void registerPPCallbacks(const SourceManager , Preprocessor *PP,
Preprocessor *ModuleExpanderPP) override;
Index: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
===
--- clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
+++ clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
@@ -34,7 +34,7 @@
 
 /// 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"; }
 
 /// Parses header file extensions from a semicolon-separated list.
 bool parseFileExtensions(StringRef AllFileExtensions,
Index: clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.h
===
--- clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.h
+++ clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.h
@@ -22,11 +22,12 @@
 /// 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
Index: clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.cpp
===
--- clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.cpp
@@ -34,7 +34,7 @@
   RawStringHeaderFileExtensions(Options.getLocalOrGlobal(
   "HeaderFileExtensions", utils::defaultHeaderFileExtensions())) {
   if (!utils::parseFileExtensions(RawStringHeaderFileExtensions,
-  HeaderFileExtensions, ',')) {
+  HeaderFileExtensions, ';')) {
 

[PATCH] D75621: [clang-tidy] Use ; as separator for HeaderFileExtensions

[Jonathan Roelofs via Phabricator via cfe-commits]

jroelofs created this revision.
jroelofs added reviewers: njames93, aaron.ballman.
Herald added subscribers: cfe-commits, xazax.hun.
Herald added a project: clang.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75621

Files:
  clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.h
  clang-tools-extra/clang-tidy/google/GlobalNamesInHeadersCheck.cpp
  clang-tools-extra/clang-tidy/google/GlobalNamesInHeadersCheck.h
  clang-tools-extra/clang-tidy/google/UnnamedNamespaceInHeaderCheck.cpp
  clang-tools-extra/clang-tidy/google/UnnamedNamespaceInHeaderCheck.h
  clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.h
  clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.cpp
  clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.h
  clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
  clang-tools-extra/clang-tidy/utils/HeaderGuard.h

Index: clang-tools-extra/clang-tidy/utils/HeaderGuard.h
===
--- clang-tools-extra/clang-tidy/utils/HeaderGuard.h
+++ clang-tools-extra/clang-tidy/utils/HeaderGuard.h
@@ -18,11 +18,12 @@
 
 /// 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)
@@ -30,7 +31,7 @@
 RawStringHeaderFileExtensions(Options.getLocalOrGlobal(
 "HeaderFileExtensions", utils::defaultHeaderFileExtensions())) {
 utils::parseFileExtensions(RawStringHeaderFileExtensions,
-   HeaderFileExtensions, ',');
+   HeaderFileExtensions, ';');
   }
   void registerPPCallbacks(const SourceManager , Preprocessor *PP,
Preprocessor *ModuleExpanderPP) override;
Index: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
===
--- clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
+++ clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
@@ -34,7 +34,7 @@
 
 /// 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"; }
 
 /// Parses header file extensions from a semicolon-separated list.
 bool parseFileExtensions(StringRef AllFileExtensions,
Index: clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.h
===
--- clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.h
+++ clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.h
@@ -22,11 +22,12 @@
 /// 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
Index: clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.cpp
===
--- clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.cpp
@@ -34,7 +34,7 @@
   RawStringHeaderFileExtensions(Options.getLocalOrGlobal(
   "HeaderFileExtensions", utils::defaultHeaderFileExtensions())) {
   if (!utils::parseFileExtensions(RawStringHeaderFileExtensions,
-  HeaderFileExtensions, ',')) {
+