MyDeveloperDay marked 6 inline comments as done. MyDeveloperDay added a comment.
I'll likely abandon this review when D68554: [clang-format] Proposal for clang-format to give compiler style warnings <https://reviews.llvm.org/D68554> lands ================ Comment at: clang/tools/clang-format/ClangFormat.cpp:245 +// Returns an invalid BOM +static const char *hasInValidBOM(StringRef BufStr) { + // Check to see if the buffer has a UTF Byte Order Mark (BOM). ---------------- owenpan wrote: > owenpan wrote: > > This code was copied from `clang/lib/Basic/SourceManager.cpp`. I suggest > > that you move this function to there and export it through > > `clang/include/clang/Basic/SourceManager.h`. > May I suggest `getInvalidBOM(const StringRef)` for the function name and > parameter type? @klimek already got me on that one over in {D68554} ================ Comment at: clang/tools/clang-format/ClangFormat.cpp:245-266 +static const char *hasInValidBOM(StringRef BufStr) { + // Check to see if the buffer has a UTF Byte Order Mark (BOM). + // We only support UTF-8 with and without a BOM right now. See + // https://en.wikipedia.org/wiki/Byte_order_mark#Byte_order_marks_by_encoding + // for more information. + const char *InvalidBOM = + llvm::StringSwitch<const char *>(BufStr) ---------------- MyDeveloperDay wrote: > owenpan wrote: > > owenpan wrote: > > > This code was copied from `clang/lib/Basic/SourceManager.cpp`. I suggest > > > that you move this function to there and export it through > > > `clang/include/clang/Basic/SourceManager.h`. > > May I suggest `getInvalidBOM(const StringRef)` for the function name and > > parameter type? > @klimek already got me on that one over in {D68554} That sounds good, do you mind if I do that separately? (as this review is a little entwined with {D68554}, which is where I'll actually update that revision with your suggestions. ================ Comment at: clang/tools/clang-format/ClangFormat.cpp:381 +// Dump the configuration. +static unsigned dumpConfig(StringRef AssumeFileName) { + StringRef FileName; ---------------- owenpan wrote: > `AssumeFileName` is a file-scope global, so the file-scope function doesn't > need to make it a parameter? Also, a `bool` or `int` may be a better return > type than `unsigned`. I think it should be `int` as it ends up being the return from main, I'll do that. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68767/new/ https://reviews.llvm.org/D68767 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
