Page MenuHomePhabricator

[clang-format] Add possibility to be based on parent directory
Needs ReviewPublic

Authored by HazardyKnusperkeks on Sun, Dec 27, 1:38 PM.

Details

Summary

This allows the define BasedOnStyle: InheritParentConfig and then clang-format looks into the parent directories for their .clang-format and takes that as a basis.

My use case is to remove the column limit for my tests.

Diff Detail

Event Timeline

HazardyKnusperkeks requested review of this revision.Sun, Dec 27, 1:38 PM
HazardyKnusperkeks created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptSun, Dec 27, 1:38 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I'm a little confused why this is needed as clang-format always read up the directory tree until it see a .clang-format file, perhaps I don't quite understand the use case. Can't you just have a different .clang-format in the subdirectory?

I'm a little confused why this is needed as clang-format always read up the directory tree until it see a .clang-format file, perhaps I don't quite understand the use case. Can't you just have a different .clang-format in the subdirectory?

Yes of course I can, but I don't want to copy everything except the one thing I want to change.

I have one .clang-format in my main directory and since I only want to change the column limit for my tests I want to write a .clang-format in the test directory with the content:

BasedOnStyle: File
ColumnLimit: 0

So when I change (or better add) a value in my main .clang-format it will also apply to my tests, without touching their file.

njames93 added a comment.EditedSun, Dec 27, 3:12 PM

I'm a little confused why this is needed as clang-format always read up the directory tree until it see a .clang-format file, perhaps I don't quite understand the use case. Can't you just have a different .clang-format in the subdirectory?

It's my understanding this is akin to the InheritParentConfig option found in .clang-tidy.
The idea is you can put a .clang-format file in your project root, then for specific directories you can add another .clang-format file that modifies that root .clang-format
The example here is a test folder that needs extended lines but want the rest of the formatting to match the root folder without having to copy the contents of the root and make the small edit.

We have it quite lucky here at LLVM where for tests we can just use:

BasedOnStyle: LLVM
ColumnLimit: 0

However for other projects with their own style they could then use:

# Copy the root directory clang.format config
BasedOnStyle: file
ColumnLimit: 0

This would also make modifying the formatting style of a project much easier as you would only need to touch the root directory .clang-format file in most cases.

With this implementation I'm not too sure on the specifics though. .clang-format files are more complex to parse than .clang-tidy files so there's more edge cases to consider.
Also it appears this wouldn't handle the case of invoking clang-format with the style set inline on the command line.
clang-format <file> --style='{BasedOnStyle: File, ColumnLimit: 80}'
Should this then check the directory for .clang-format files?

clang/lib/Format/Format.cpp
2980

This seems a very inefficient way of doing this, getStyle does a lot of things we simple don't need to worry about. Maybe the file loading part should be extracted out into its own function, that can then recursively call itself. Or use the clang-tidy approach of storing configurations in a stack (will have to be unparsed cause how clang-format works) and then parse them on top of each other.

I'm a little confused why this is needed as clang-format always read up the directory tree until it see a .clang-format file, perhaps I don't quite understand the use case. Can't you just have a different .clang-format in the subdirectory?

It's my understanding this is akin to the InheritParentConfig option found in .clang-tidy.
The idea is you can put a .clang-format file in your project root, then for specific directories you can add another .clang-format file that modifies that root .clang-format
The example here is a test folder that needs extended lines but want the rest of the formatting to match the root folder without having to copy the contents of the root and make the small edit.

We have it quite lucky here at LLVM where for tests we can just use:

BasedOnStyle: LLVM
ColumnLimit: 0

However for other projects with their own style they could then use:

# Copy the root directory clang.format config
BasedOnStyle: file
ColumnLimit: 0

This would also make modifying the formatting style of a project much easier as you would only need to touch the root directory .clang-format file in most cases.

Exactly.

With this implementation I'm not too sure on the specifics though. .clang-format files are more complex to parse than .clang-tidy files so there's more edge cases to consider.
Also it appears this wouldn't handle the case of invoking clang-format with the style set inline on the command line.
clang-format <file> --style='{BasedOnStyle: File, ColumnLimit: 80}'
Should this then check the directory for .clang-format files?

Initially I thought it shouldn't. But now I think maybe it should.

clang/lib/Format/Format.cpp
2980

Yeah I know that this is double work, but I thought it only affects the case BasedOnStyle: File and keeps the "normal" way untouched.

If I do build a stack I would actually parse all .clang-format files up to the root directory and in the not BasedOnStyle: File case throw away the results, wouldn't I? The question is how often someone has a .clang-fomrat in one of his parent directories which is not used because it got overwritten.

But I can do that.

Or is there an easy way to just check the BasedOnStyle value? As far as I understood the yaml code it parses the whole file into an internal structure and we only query this structure, right?

MyDeveloperDay added a comment.EditedTue, Dec 29, 3:42 AM

I like what you are suggesting, do you think BasedOnStyle: File is the best terminology as the term File is used elsewhere to mean read from the .clang_format file, how about

BasedOnStyle: Inherit
BasedOnStyle: InheritFromParent
BasedOnStyle: InheritFromFile

or why not just

BasedOnStyle: InheritParentConfig

to match clang-tidy?

I like what you are suggesting, do you think BasedOnStyle: File is the best terminology as the term File is used elsewhere to mean read from the .clang_format file, how about

BasedOnStyle: Inherit
BasedOnStyle: InheritFromParent
BasedOnStyle: InheritFromFile

or why not just

BasedOnStyle: InheritParentConfig

to match clang-tidy?

I will update it to InheritParentConfig.

njames93 added inline comments.Wed, Dec 30, 6:56 AM
clang/lib/Format/Format.cpp
2980

Yeah I know that this is double work, but I thought it only affects the case BasedOnStyle: File and keeps the "normal" way untouched.

If I do build a stack I would actually parse all .clang-format files up to the root directory and in the not BasedOnStyle: File case throw away the results, wouldn't I? The question is how often someone has a .clang-fomrat in one of his parent directories which is not used because it got overwritten.

I'm not saying start at the route directory and work down to the child. Still start at the child, but when you parse a configuration that has needs the parent, put that unparsed config into a stack, then carry on. At the end then you can join all those configurations

But I can do that.

Or is there an easy way to just check the BasedOnStyle value? As far as I understood the yaml code it parses the whole file into an internal structure and we only query this structure, right?

It should be possible, not an expert but I'm not sure it like being traversed multiple times normally, though a workaround should be feasible.

HazardyKnusperkeks marked 2 inline comments as done.
HazardyKnusperkeks edited the summary of this revision. (Show Details)
  • Rebased
  • Renamed to InhteritParentConfig
  • Reworked to remove the recursion of getStyle
  • Now supports the command line style options
  • Test cases extended for the latter

What I can't easily tell from the tests is if you are overriding any styles defined in the parent with a local style.

clang/include/clang/Format/Format.h
57

I don't quite understand this last sentence

What I can't easily tell from the tests is if you are overriding any styles defined in the parent with a local style.

Yes I do, but I will try to make it clearer in the tests.

clang/include/clang/Format/Format.h
57

If I would use /// it would end up in the ClangFormatStyleOptions.rst. I only want to make clear it is no mistake that it only got // while all other members are documented with ///.

What I can't easily tell from the tests is if you are overriding any styles defined in the parent with a local style.

Yes I do, but I will try to make it clearer in the tests.

Okay after rechecking the code (it is a bit ago that I wrote it), what more do you need? I compare the parsed style against a base style (Google, to show that it’s not the default LLVM) modified with the values from the overwrite. Only for the last checks where I basically check the same over and over again, just trough different means I use a stored style.