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:
> 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;
}
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75621/new/
https://reviews.llvm.org/D75621
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
