zwliew added inline comments.
================
Comment at: clang/lib/Format/Format.cpp:3067
+ if (!ChildFormatTextToApply.empty()) {
+ assert(ChildFormatTextToApply.size() == 1);
+
----------------
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!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93844/new/
https://reviews.llvm.org/D93844
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
