[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 &FileExtensions, char Delimiter);
+                         FileExtensionsSet &FileExtensions,
+                         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<FileExtensionSet>`.
> > 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;
> > }
> > ```
> 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