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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75621