[PATCH] D93844: [clang-format] Add possibility to be based on parent directory

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks marked an inline comment as done.
HazardyKnusperkeks added inline comments.


================
Comment at: clang/lib/Format/Format.cpp:3067
+  if (!ChildFormatTextToApply.empty()) {
+    assert(ChildFormatTextToApply.size() == 1);
+
----------------
zwliew wrote:
> zwliew wrote:
> > HazardyKnusperkeks wrote:
> > > zwliew wrote:
> > > > HazardyKnusperkeks wrote:
> > > > > zwliew wrote:
> > > > > > Is there a reason behind limiting the number of children configs to 1 in this case? When the fallback is not used, more than 1 children configs are allowed (line 3036).
> > > > > > 
> > > > > > Sorry for digging this up, I came across this seemingly arbitrary decision when working on some changes to https://reviews.llvm.org/D72326
> > > > > Yeah but it doesn't happen, there is at most only one `.clang-format` in the parent directory path which is found. So we assert on that and then don't need to loop over what is exactly one element big.
> > > > Thanks for the reply. However, I don't think that's true. Yes, it's only possible to find one `.clang-format` file in the first parent directory. But if it has `BasedOnStyle: InheritParentConfig` set, then we could find another `.clang-format` file in the "grandparent" directory. In this case, we'll have 2 elements in `ChildFormatTextToApply`, but only the very first one will actually be applied.
> > > > 
> > > > To illustrate, suppose we have the following file structure:
> > > > ```
> > > > - .clang-format
> > > > - foo
> > > >   |- .clang-format
> > > >   |- input.cpp
> > > > ```
> > > > 
> > > > Both `.clang-format` files have `BasedOnStyle: InheritParentConfig` set. When running `clang-format --style=file foo/input.cpp`, only the inner config is applied on the fallback style, while the outer config is ignored. When testing on a debug build, I encountered a crash due to the failed assert. When removing the assert, and using a loop to apply the configs, both the inner and outer configs are applied, which I believe is the expected behaviour.
> > > > 
> > > I can not explain it to you anymore, I would have to dig into it again. But if my tests are correct everything works. You can see that for `Test 9.4` I create a `.clang-format` in `/e/sub/sub/` which is based on `/e/sub/.clang-format` which again is based on `/e/.clang-format`.
> > > 
> > > The tests do not fail, the parsed style is as the three files suggest, and the assert holds. I'm pretty sure I have thought about that case, it happens in some kind of recursion.
> > I took a look through `Test 9.4`, and it doesn't test the case I'm thinking of, as it doesn't execute the fallback case.
> > 
> > In ` Test 9.4` the file structure is as follows:
> > ```
> > - e/
> >   |- .clang-format (BasedOnStyle: Google) <-- outermost config
> >   |- sub/
> >     |- .clang-format (BasedOnStyle: `InheritParentConfig)
> >     |- sub/
> >       |- .clang-format (BasedOnStyle: InheritParentConfig)
> >       |- code.cpp
> > ```
> > 
> > The reason it doesn't execute the fallback case is that the outermost config file doesn't have `BasedOnStyle: InheritParentConfig` set.
> > 
> > On the other hand, in the following directory structure, the fallback case would execute, the assert would fail (on debug builds), and only the innermost config would be applied (on release builds):
> > ```
> > - e/
> >   |- .clang-format (BasedOnStyle: InheritParentConfig) <-- outermost config
> >   |- sub/
> >     |- .clang-format (BasedOnStyle: `InheritParentConfig)
> >     |- sub/
> >       |- .clang-format (BasedOnStyle: InheritParentConfig)
> >       |- code.cpp
> > ```
> > 
> > In order to verify my claims, I think I should write a new test case for this. However, I do not know how to run test cases for only clang-format. Is there a way to do so? Thanks! 
> I added a new test case for the latter scenario and ran all the regression test cases with `make check-clang`. The test case does indeed fail due to the assertion failure. I've updated https://reviews.llvm.org/D72326 with the test case and the fix for it. Please have a look, thanks!
In that case thanks for catching that.

But I think it should be a different patch for the fix. Do not put a bug fix with a new feature in one commit.
There should be a target FormatTests, which builds and runs only the tests for clang-format.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93844/new/

https://reviews.llvm.org/D93844