zwliew added inline comments.
================
Comment at: clang/lib/Format/Format.cpp:3067
+ if (!ChildFormatTextToApply.empty()) {
+ assert(ChildFormatTextToApply.size() == 1);
+
----------------
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!
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
