Page MenuHomePhabricator

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

Authored by HazardyKnusperkeks on Dec 27 2020, 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

Unit TestsFailed

TimeTest
280 msx64 debian > libarcher.races::task-dependency.c
Script: -- : 'RUN: at line 13'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -Wl,-rpath,/mnt/disks/ssd0/agent/llvm-project/build/lib /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-dependency.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-dependency.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/deflake.bash /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-dependency.c.tmp 2>&1 | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-dependency.c.tmp.log | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-dependency.c
280 msx64 debian > libarcher.races::task-taskgroup-unrelated.c
Script: -- : 'RUN: at line 13'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -Wl,-rpath,/mnt/disks/ssd0/agent/llvm-project/build/lib /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-taskgroup-unrelated.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-taskgroup-unrelated.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/deflake.bash /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-taskgroup-unrelated.c.tmp 2>&1 | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-taskgroup-unrelated.c.tmp.log | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-taskgroup-unrelated.c
290 msx64 debian > libarcher.races::task-taskwait-nested.c
Script: -- : 'RUN: at line 13'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -Wl,-rpath,/mnt/disks/ssd0/agent/llvm-project/build/lib /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-taskwait-nested.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-taskwait-nested.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/deflake.bash /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-taskwait-nested.c.tmp 2>&1 | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-taskwait-nested.c.tmp.log | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-taskwait-nested.c
260 msx64 debian > libarcher.races::task-two.c
Script: -- : 'RUN: at line 13'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -Wl,-rpath,/mnt/disks/ssd0/agent/llvm-project/build/lib /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-two.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-two.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/deflake.bash /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-two.c.tmp 2>&1 | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-two.c.tmp.log | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/races/task-two.c
280 msx64 debian > libarcher.task::task-barrier.c
Script: -- : 'RUN: at line 15'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -Wl,-rpath,/mnt/disks/ssd0/agent/llvm-project/build/lib /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/task/task-barrier.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/task/Output/task-barrier.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/task/Output/task-barrier.c.tmp 2>&1 | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/tools/archer/tests/task/Output/task-barrier.c.tmp.log | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/tools/archer/tests/task/task-barrier.c
View Full Test Results (13 Failed)

Event Timeline

HazardyKnusperkeks requested review of this revision.Dec 27 2020, 1:38 PM
HazardyKnusperkeks created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 27 2020, 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.EditedDec 27 2020, 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
3014

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
3014

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.EditedDec 29 2020, 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.Dec 30 2020, 6:56 AM
clang/lib/Format/Format.cpp
3014

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.

Found and fixed the case when no BasedOnStyle apart from InheritParentConfig is used.

Rebased and Ping.

curdeius added inline comments.Feb 1 2021, 9:35 AM
clang/docs/ClangFormatStyleOptions.rst
160

Typo: fount -> found.

160

Why should it fallback to LLVM and not the fallback style?
It seems to me that it's a documentation problem.

HazardyKnusperkeks marked 3 inline comments as done.Feb 1 2021, 1:38 PM
HazardyKnusperkeks added inline comments.
clang/docs/ClangFormatStyleOptions.rst
160

Yap, fixed that.

HazardyKnusperkeks marked an inline comment as done.

Fixed documentation (and rebased).

MyDeveloperDay accepted this revision.Feb 2 2021, 1:32 AM
This revision is now accepted and ready to land.Feb 2 2021, 1:32 AM

As a follow up it may be wise to pass a diag handler to parseConfiguration as when we parse it a second time, we probably want to disregard any warnings (like unknown key) detected as they will have been printed on the first pass.

clang/lib/Format/Format.cpp
3011–3017

Nit: just use a for loop over a lambda.

3051

Nit: Add a void cast to silence unused warnings in release builds.

HazardyKnusperkeks marked 2 inline comments as done.Feb 2 2021, 12:40 PM

As a follow up it may be wise to pass a diag handler to parseConfiguration as when we parse it a second time, we probably want to disregard any warnings (like unknown key) detected as they will have been printed on the first pass.

Will try to look into it.

clang/lib/Format/Format.cpp
3011–3017

Did not know about llvm::reverse.

HazardyKnusperkeks marked an inline comment as done.

Silence unused variable warnings in release and using llvm::reverse with range based for.

This revision was landed with ongoing or failed builds.Feb 14 2021, 10:56 AM
This revision was automatically updated to reflect the committed changes.