Page MenuHomePhabricator

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

Authored by MyDeveloperDay on Jan 7 2020, 5:39 AM.



Rebase original diff of 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!

Diff Detail

Event Timeline

tnorth created this revision.Jan 7 2020, 5:39 AM
tnorth retitled this revision from Rebased on master: Add option to specify explicit config file to [clang-format] Rebased on master: Add option to specify explicit config file.
tnorth edited the summary of this revision. (Show Details)
tnorth added reviewers: djasper, ioeric, krasimir.

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.
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

2573 ↗(On Diff #236561)

Expected<FormatStyle> or ErrorOr<FormatStyle> would be a better return type for this function I think.
Read isn't going to return unsuitable, so plumbing through multiple error codes doesn't seem worthwhile.

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?

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)

tnorth updated this revision to Diff 238513.Jan 16 2020, 8:46 AM

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.

Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2020, 8:46 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
MyDeveloperDay added a project: Restricted Project.Jan 19 2020, 8:04 AM
tnorth updated this revision to Diff 240888.Jan 28 2020, 7:40 AM
  • Add a unit-test loading a format and test file from temporary files.
tnorth marked 7 inline comments as done.Jan 28 2020, 7:46 AM

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).

tnorth updated this revision to Diff 241155.Jan 29 2020, 7:14 AM
  • Add release notes
  • Update ClangFormat.rst and ClangFormatStyleOption.rst

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/, 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.

MyDeveloperDay accepted this revision.Feb 5 2020, 12:11 PM

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.

This revision is now accepted and ready to land.Feb 5 2020, 12:11 PM

Ping. I guess we need more approval to go forward?

lebedev.ri added inline comments.
2341 ↗(On Diff #241155)

So is it : or =?

tnorth updated this revision to Diff 245592.Feb 20 2020, 12:15 AM

Fix comment to file:<configfile> instead of the incorrect file=<configfile>

tnorth marked 2 inline comments as done.Feb 20 2020, 12:16 AM
tnorth added inline comments.
2341 ↗(On Diff #241155)

: is correct, fixed.

tnorth marked an inline comment as done.Feb 20 2020, 12:43 AM
tnorth added a comment.Mar 6 2020, 6:52 AM

Thanks @MyDeveloperDay . I guess more approvals are needed at this point?

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.

tnorth updated this revision to Diff 249616.Mar 11 2020, 7:19 AM

Rebased on master (6d5603e2). Some refactoring was indeed needed.

This revision was automatically updated to reflect the committed changes.
mitchell-stellar reopened this revision.Mar 11 2020, 1:18 PM

Reverted this commit due to an unexpected test failure:

******************** TEST 'Clang :: Format/dump-config-objc.h' FAILED ********************
: '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.

This revision is now accepted and ready to land.Mar 11 2020, 1:18 PM

Ping @MyDeveloperDay , any clue about what's going on here?

MyDeveloperDay added inline comments.Apr 7 2020, 7:05 AM

if you return here IsSuitable will be true..


again here you need to set IsSutable to false


maybe its better just to set this to true here *IsSuitable = true; and set it to false on line @2695


this could then mean you return with something wrong here..

MyDeveloperDay added inline comments.Apr 16 2020, 2:14 AM

I think this isn't needed won't it already be false from line @2695

tnorth updated this revision to Diff 258029.Apr 16 2020, 5:56 AM

Were you able to rerun these tests?

MyDeveloperDay added a comment.EditedApr 30 2020, 4:41 AM

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/ -v ./tools/clang/test/Format

$ ./ C:\cygwin64\buildareas\clang\build\bin\..\..\llvm-project\llvm\utils\lit\lit\llvm\ 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 ********************
: '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


MyDeveloperDay requested changes to this revision.Apr 30 2020, 5:15 AM
MyDeveloperDay added inline comments.

pass Style in, don't keep assuming LLVM


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 =
  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 + ": " +
  *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) + ": " +
  return FileStyle;

And then in the loop of files you pass the Style in too
if (auto ConfigStyle =

LoadConfigFile(ConfigFile, FS, &IsSuitable, Style)) {

pass Style in and rename return value


We should probably keep something like this so we know which config file we are loading


Pass current Style in

This revision now requires changes to proceed.Apr 30 2020, 5:15 AM

ping @tnorth are you planning on fixing this as its currently reverted.

@tnorth Are you planning on working on this or do you mind if one of us fixes the issues enough to get it over the line. (this was previously accepted and landed but the unit tests fail, for what looks like a fairly minor issue)

MyDeveloperDay edited reviewers, added: HazardyKnusperkeks, curdeius, owenpan; removed: Restricted Project, djasper.Wed, Nov 24, 3:30 AM

After rebasing these tests seem to work (on Windows)

-- Testing: 24 tests, 16 workers --
PASS: Clang :: Format/dry-run-alias.cpp (1 of 24)
PASS: Clang :: Format/remove-duplicate-includes.cpp (2 of 24)
PASS: Clang :: Format/disable-format.cpp (3 of 24)
PASS: Clang :: Format/cursor.cpp (4 of 24)
PASS: Clang :: Format/multiple-inputs.cpp (5 of 24)
PASS: Clang :: Format/basic.cpp (6 of 24)
PASS: Clang :: Format/incomplete.cpp (7 of 24)
PASS: Clang :: Format/line-ranges.cpp (8 of 24)
PASS: Clang :: Format/adjust-indent.cpp (9 of 24)
UNSUPPORTED: Clang :: Format/style-on-command-line.cpp (10 of 24)
PASS: Clang :: Format/ranges.cpp (11 of 24)
PASS: Clang :: Format/access-modifiers.cpp (12 of 24)
PASS: Clang :: Format/multiple-inputs-error.cpp (13 of 24)
PASS: Clang :: Format/dump-config-objc.h (14 of 24)
PASS: Clang :: Format/xmloutput.cpp (15 of 24)
PASS: Clang :: Format/dump-config-cxx.h (16 of 24)
PASS: Clang :: Format/multiple-inputs-inplace.cpp (17 of 24)
PASS: Clang :: Format/error-config.cpp (18 of 24)
PASS: Clang :: Format/dry-run.cpp (19 of 24)
PASS: Clang :: Format/inplace.cpp (20 of 24)
PASS: Clang :: Format/dump-config-list-override.cpp (21 of 24)
PASS: Clang :: Format/disable-include-sorting.cpp (22 of 24)
PASS: Clang :: Format/language-detection.cpp (23 of 24)
PASS: Clang :: Format/verbose.cpp (24 of 24)

I'm going to Commandeer this revision and see if we can't get this relanded.

MyDeveloperDay commandeered this revision.Wed, Nov 24, 6:32 AM
MyDeveloperDay edited reviewers, added: tnorth; removed: MyDeveloperDay.
This revision now requires review to proceed.Wed, Nov 24, 6:32 AM

Rebasing previously landed but reverted commit


part of me wonders if this format should be

file://<path> rather than file:<path>

User provided clang-format file using -style=file:///path/to/format/file


User provided clang-format file using -style=file:/path/to/format/file

This would leave the way open to other protocols http:// or https:// or anything else.


Maybe, but I doubt we want http. And if we use file:// a windows path D:\Path\ would not be valid, would it?

MyDeveloperDay added a comment.EditedThu, Nov 25, 7:40 AM

The reason I have picked this us was because of:

This slightly annoys me because :

a) What they were talking about was in my view is disrespectful and inaccurate.
b) I thought we'd already landed this (which we had)

I went looking for this review which had previously been accepted and landed, but got reverted because it seemed to fail the tests

The original author and the original-original-author has both obviously moved on and dropped it and so it didn't get fixed.

I don't like wasting all that effort, especailly if we are going to get grief for it. So I rebased the review so we can land it again, (and checked both the unit tests and lit tests)

I hope we don't have to go around the houses on this too much.


yes file://C:\Windows\Kernel32.dll is a valid file url.

but I on reflection I think file: will be ok.

Hi there,
Yes, sorry for the long silence, I ended up not having time to come back to this anymore for personal reasons. I also lack familiarity with the codebase and tests, and I couldn't invest more time into it; I had the impression that this last failing test would require major changes.
I'm happy if this ends up being merged. Thanks for your time and guidance again.