[PATCH] D151047: [clang-format] Fix indentation for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono added a comment.

Heh, ok, so I wasn't that naive then to not run the tests of everything :-)

I had a look at the issue. The ClangRenameTests first do some replacements, and then call formatAndApplyReplacements() <https://github.com/llvm/llvm-project/blob/9598fe54e216d78da017f804382e9971918fff2d/clang/include/clang/Tooling/Refactoring.h#L92> with them, which also formats the lines containing the replacements. With my patch, calling `clang-format.exe -style=llvm --lines=13:13` on the following file (line 13 is the `OldAlias old_alias;`)

  #include "header.h"
  
      namespace x { class Old {}; }
      namespace ns {
      #define REF(alias) alias alias_var;
  
      #define ALIAS(old) \
        using old##Alias = x::old; \
        REF(old##Alias);
  
      ALIAS(Old);
  
      OldAlias old_alias;
      }

triggers the assert in adjustToUnmodifiedLine() <https://github.com/llvm/llvm-project/blob/9598fe54e216d78da017f804382e9971918fff2d/clang/lib/Format/UnwrappedLineFormatter.cpp#L94>. This is what happens in the `RenameAliasTest.AliasesInMacros` test.

The issue originates already from the line `#define REF(alias) alias alias_var;`: These are two `AnnotatedLine` instances at first, namely `#define REF(alias)` with `AnnotatedLine::Level` set to 0 and `alias alias_var;` with `AnnotatedLine::Level` equals to 1. They get joined eventually. Since my patch sets `A.Level = B.Level;` in `join()`, the joined line gets level 1. But the `LevelIndentTracker` is still in level 0 (i.e. `IndentForLevel` contains only 1 element).

It is kind of the same problem why I added the `if (!Line.InPPDirective)` check in `nextLine()` in the patch: I think, if both AnnotatedLines are PP directives, then `join()` should not step "into" or "out of" levels. PP-directives are kind-of "temporary insertions". So, replacing in `join()` the `A.Level = B.Level;` from my patch with

  assert(A.InPPDirective == B.InPPDirective);
  if (!A.InPPDirective && !B.InPPDirective)
    A.Level = B.Level;

seems to fix the problem. At least all Google tests (including the ClangRenameTests) pass that didn't fail without the patch. Any opinions on that idea?

I have to think some more about the levels and what it means regarding `join()`. I also need to get the clang-tidy test to run locally and also add the above example as a formatter test (if possible in a reduced form). Most likely I won't be able to do this until next week, sorry. :-/

To create a new fix, do I understand the guide <https://llvm.org/docs/Phabricator.html#using-patch-summaries> correctly that I basically execute `arc diff --verbatim` and mention `Depends on D151047` somewhere in the message? Will it then appear here automatically?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151047