[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Zhao Wei Liew via Phabricator via cfe-commits]

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