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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
