zwliew added a comment. Hi, I'd like to help to get this patch accepted and merged. I have a few suggestions/questions below, and I can help make any changes to the patch if needed!
================ Comment at: clang/docs/ClangFormatStyleOptions.rst:35 +When using ``-style=file:<format_file_path>, :program:`clang-format` for +each input file will use the format file located at `<format_file_path>`. ---------------- I think two backticks are missing here. ================ Comment at: clang/include/clang/Format/Format.h:4068 /// directory if ``FileName`` is empty. +/// * "file:<configfile>" to explicitly specify the configuration file to use. /// ---------------- To be consistent with the `ClangFormatOptions.rst` docs, I think `configfile` should be changed to `format_file_path`. ================ Comment at: clang/lib/Format/Format.cpp:3166 "directory for stdin).\n" + "Use -style=file:<configfile> to explicitly specify" + "the configuration file.\n" ---------------- Similar to above, `configfile` should be changed to `format_file_path`. ================ Comment at: clang/lib/Format/Format.cpp:3219 + llvm::vfs::FileSystem *FS, + bool *IsSuitable) { + FormatStyle Style = getLLVMStyle(); ---------------- I think `LoadConfigFile` could be refactored to return `std::error_code` instead, similar to `parseConfiguration()`, like so: ``` // note I renamed LoadConfigFile to loadConfigFile to follow the existing function naming style. std::error_code loadConfigFile(StringRef ConfigFile, llvm::vfs::FileSystem *FS, FormatStyle *Style) { llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> Text = FS->getBufferForFile(ConfigFile.str()); if (auto ec = Text.getError()) { return ec; } return parseConfiguration(Text.get()->getBuffer(), Style); } ``` And the part in `getStyle()` would look like this: ``` // Check for explicit config filename if (StyleName.startswith_insensitive("file:")) { auto StyleFileName = StyleName.substr(5); if (auto ec = loadConfigFile(StyleFileName, FS, &Style)) { return make_string_error(ec.message()); } LLVM_DEBUG(llvm::dbgs() << "Using configuration file " << StyleFileName << '\n'); return Style; } ``` What do you think? ================ Comment at: clang/lib/Format/Format.cpp:3273 + llvm::SmallVector<std::string, 2> FilesToLookFor; + // User provided clang-format file using -style=file:/path/to/format/file ---------------- HazardyKnusperkeks wrote: > Why move that, it it's not used here? Yup, I think this shouldn't be moved. ================ Comment at: clang/lib/Format/Format.cpp:3276 + // Check for explicit config filename + if (StyleName.startswith_insensitive("file:")) { + auto StyleNameFile = StyleName.substr(5); ---------------- Following my code suggestion above, I think this code block could be refactored to this: ``` // Check for explicit config filename if (StyleName.startswith_insensitive("file:")) { auto StyleFileName = StyleName.substr(5); if (auto ec = loadConfigFile(StyleFileName, FS, &Style)) { return make_string_error(ec.message()); } LLVM_DEBUG(llvm::dbgs() << "Using configuration file " << StyleFileName << '\n'); return Style; } ``` What do you think? Also, on another note, I feel like the `-style` option is too overloaded. Would it be better to use a separate option like `-style-file` instead? ================ Comment at: clang/lib/Format/Format.cpp:2693 + llvm::vfs::FileSystem *FS, + bool *IsSuitable) { + FormatStyle Style = getLLVMStyle(); ---------------- MyDeveloperDay wrote: > pass Style in, don't keep assuming LLVM I think it's still a good idea to pass Style into the function instead. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72326/new/ https://reviews.llvm.org/D72326 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits