Rebase original diff of https://reviews.llvm.org/D33560 onto master. I also tried to address the remaining comments.
This is a quick attempt to get this feature into clang-format - I hope that the tiny restructuring (new function) is acceptable in this form: comment & improvements welcome!
Details
- Reviewers
djasper ioeric krasimir MyDeveloperDay - Group Reviewers
Restricted Project - Commits
- rG10b1a87ba35d: [clang-format] Add option to specify explicit config file Summary: This diff…
Diff Detail
- Repository
- rC Clang
Event Timeline
In principle, I think this is something that might help a lot of people as I've seen people asking for a better mechanism to be able to load a centrally stored .clang-format file.
but maybe wait a couple of days for others to comment @klimek , @mitchell-stellar , @sammccall ...
Also you need to make full context diffs, plus please add a release note, and because I can't check the full diff, I'm unsure if your Format.h change means we need to regenerate anything in the ClangFormatStyleOptions.rst
In this patch, relative paths are relative to the working directory (or at least the current directory of the VFS).
This makes sense for command-line args, but if I understand correctly this patch will also allow BasedOnStyle: file:some/path. Is that the case?
In that case, interpreting paths relative to the working directory seems suspicious. I'd suggest one of:
- BasedOnStyle: file:... is not allowed
- BasedOnStyle: file:... must be an absolute path (note this probably means it's not portable across OSs)
- BasedOnStyle: file:relative/path is relative to the current config file, or working directory if the current config file is inline in command line args.
include/clang/Format/Format.h | ||
---|---|---|
2329 ↗ | (On Diff #236561) | Please upload using full context (e.g. with arc diff) |
2332 ↗ | (On Diff #236561) | why does this need to be a public API? |
2343 ↗ | (On Diff #236561) | this function isn't implemented - LoadConfigFile vs loadConfigFile |
lib/Format/Format.cpp | ||
2573 ↗ | (On Diff #236561) | Expected<FormatStyle> or ErrorOr<FormatStyle> would be a better return type for this function I think. |
2613 ↗ | (On Diff #236561) | just slice the stringref rather than copy here? |
2615 ↗ | (On Diff #236561) | why stat the file and check, rather than just try to read it? |
unittests/Format/FormatTest.cpp | ||
14370 ↗ | (On Diff #236561) | Can you add this via a real absolute path, to show this is actually relative path handling (and not just naive string manipulation by memory file system etc) |
Thanks for your time and feedback.
- Update diff with context,
- remove public declaration of LoadConfigFile,
- change prototype of LoadConfigFile and simplify it,
- avoid string copy to ConfigFile, use StringRef directly,
- don't stat the file before attempting to read it.
Regarding the tests, I am unsure how to do this currenlty (I'd need to read some docs/other examples).
I didn't have in mind the use case of BasedOnStyle: file:some/path.
This makes sense for command-line args, but if I understand correctly this patch will also allow BasedOnStyle: file:some/path. Is that the case?
No, it should not, and I also think it's better not to.
I think that all points are addressed now. Looking forward to have your feedback.
Nit: please add a release note and regenerate the ClangFormatStyleOptions.rst (if there are any changes because you modified Format.h).
Hmm, I tried to run docs/tools/dump_format_style.py, but it fails at two locations (once because a comment in Format.h has two slashes instead of 3, one because of an empty line). After fixing those, it seems that the generated file contains less information than the commited one. This file was probably updated manually as well. Therefore I also did in the updated diff... one should probably fix this separately.
Release note + ClangFormat.rst files update in the diff as well.
In the absence of any other comments, This LGTM, this may help to resolve D68569: [clang-format] Also look for .{ext}.clang-format file, I think I prefer this solution.
include/clang/Format/Format.h | ||
---|---|---|
2341 ↗ | (On Diff #241155) | So is it : or =? |
include/clang/Format/Format.h | ||
---|---|---|
2341 ↗ | (On Diff #241155) | : is correct, fixed. |
It's not more approval that is needed, it's just that someone with commit access (assuming you do not) needs to find the time to commit this. For what it's worth, I'm getting a patch rejection for the 4th block in lib/Format/Format.cpp. It seems the contents of LoadConfigFile need to be updated to reflect the most recent changes, so please rebase against master when you can.
Reverted this commit due to an unexpected test failure:
******************** TEST 'Clang :: Format/dump-config-objc.h' FAILED ******************** Script: -- : 'RUN: at line 1'; /b/1/clang-x86_64-debian-fast/llvm.obj/bin/clang-format -dump-config /b/1/clang-x86_64-debian-fast/llvm.src/clang/test/Format/dump-config-objc.h | /b/1/clang-x86_64-debian-fast/llvm.obj/bin/FileCheck /b/1/clang-x86_64-debian-fast/llvm.src/clang/test/Format/dump-config-objc.h -- Exit Code: 1 Command Output (stderr): -- /b/1/clang-x86_64-debian-fast/llvm.src/clang/test/Format/dump-config-objc.h:3:11: error: CHECK: expected string not found in input // CHECK: Language: ObjC ^ <stdin>:1:1: note: scanning from here --- ^ <stdin>:2:1: note: possible intended match here Language: Cpp ^ -- ********************
I don't know enough about this patch in order to determine what the issue is, or how to proceed further. Perhaps @MyDeveloperDay will chime in.
clang/lib/Format/Format.cpp | ||
---|---|---|
2704 | I think this isn't needed won't it already be false from line @2695 |
These tests still fail
running the following in the build directory (if your build directory is side-by-side with the llvm-project directory):
c:/Python37/python ./bin/llvm-lit.py -v ./tools/clang/test/Format
$ ./run_format_lit_tests.sh llvm-lit.py: C:\cygwin64\buildareas\clang\build\bin\..\..\llvm-project\llvm\utils\lit\lit\llvm\config.py:343: note: using clang: c:\cygwin64\buildareas\clang\build\bin\clang.exe -- Testing: 21 tests, 12 workers -- PASS: Clang :: Format/cursor.cpp (1 of 21) FAIL: Clang :: Format/dump-config-objc.h (2 of 21) ******************** TEST 'Clang :: Format/dump-config-objc.h' FAILED ******************** Script: -- : 'RUN: at line 1'; c:\cygwin64\buildareas\clang\build\bin\clang-format.exe -dump-config C:\cygwin64\buildareas\clang\llvm-project\clang\test\Format\dump-config-objc.h | c:\cygwin64\buildareas\clang\build\bin\filecheck.exe C:\cygwin64\buildareas\clang\llvm-project\clang\test\Format\dump-config-objc.h -- Exit Code: 1 Command Output (stdout): -- $ ":" "RUN: at line 1" $ "c:\cygwin64\buildareas\clang\build\bin\clang-format.exe" "-dump-config" "C:\cygwin64\buildareas\clang\llvm-project\clang\test\Format\dump-config-objc.h" $ "c:\cygwin64\buildareas\clang\build\bin\filecheck.exe" "C:\cygwin64\buildareas\clang\llvm-project\clang\test\Format\dump-config-objc.h" # command stderr: C:\cygwin64\buildareas\clang\llvm-project\clang\test\Format\dump-config-objc.h:3:11: error: CHECK: expected string not found in input // CHECK: Language: ObjC ^ <stdin>:1:1: note: scanning from here --- ^ <stdin>:2:1: note: possible intended match here Language: Cpp ^ error: command failed with exit status: 1 -- ********************
clang/lib/Format/Format.cpp | ||
---|---|---|
2693 | pass Style in, don't keep assuming LLVM | |
2694 | You cannot keep assuming the style is LLVM, Style needs to be passed in /// Attempts to load a format file llvm::Expected<FormatStyle> LoadConfigFile(StringRef ConfigFile, llvm::vfs::FileSystem *FS, bool *IsSuitable, FormatStyle Style) { *IsSuitable = false; llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> Text = FS->getBufferForFile(ConfigFile.str()); if (std::error_code EC = Text.getError()) return make_string_error(EC.message()); std::error_code ParserErrorCode = parseConfiguration(Text.get()->getBuffer(), &Style); if (ParserErrorCode == ParseError::Unsuitable) { return Style; } else if (ParserErrorCode != ParseError::Success) { return make_string_error("Error reading " + ConfigFile + ": " + ParserErrorCode.message()); } *IsSuitable = true; return Style; } Now your code will read // User provided clang-format file using -style=file:/path/to/format/file // Check for explicit config filename if (StyleName.startswith_lower("file:")) { auto StyleNameFile = StyleName.substr(5); bool IsSuitable = true; auto FileStyle = LoadConfigFile(StyleNameFile, FS, &IsSuitable, Style); if (FileStyle && !IsSuitable) { return make_string_error("Configuration file(s) do(es) not support " + getLanguageName((*FileStyle).Language) + ": " + StyleNameFile); } return FileStyle; } ` And then in the loop of files you pass the Style in too LoadConfigFile(ConfigFile, FS, &IsSuitable, Style)) { | |
2743 | pass Style in and rename return value | |
2763 | We should probably keep something like this so we know which config file we are loading | |
2785 | Pass current Style in |