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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
