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