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

Jonathan Roelofs via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Mar 9 08:03:26 PDT 2020


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





More information about the cfe-commits mailing list