[PATCH] D137181: [clang-format] Don't use 'PPIndentWidth' inside multi-line macros

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n added a comment.

In D137181#3918673 <https://reviews.llvm.org/D137181#3918673>, @owenpan wrote:

> In D137181#3918117 <https://reviews.llvm.org/D137181#3918117>, @goldstein.w.n wrote:
>
>>>> maybe you did something different?
>>>
>>> Here is what I did:
>>>
>>>   $ git diff UnwrappedLineParser.cpp
>>>   diff --git a/clang/lib/Format/UnwrappedLineParser.cpp b/clang/lib/Format/UnwrappedLineParser.cpp
>>>   index 25d9018fa109..ab3b9c53ee54 100644
>>>   --- a/clang/lib/Format/UnwrappedLineParser.cpp
>>>   +++ b/clang/lib/Format/UnwrappedLineParser.cpp
>>>   @@ -197,6 +197,7 @@ public:
>>>        PreBlockLine = std::move(Parser.Line);
>>>        Parser.Line = std::make_unique<UnwrappedLine>();
>>>        Parser.Line->Level = PreBlockLine->Level;
>>>   +    Parser.Line->PPLevel = PreBlockLine->PPLevel;
>>>        Parser.Line->InPPDirective = PreBlockLine->InPPDirective;
>>>        Parser.Line->InMacroBody = PreBlockLine->InMacroBody;
>>>      }
>>>   @@ -1274,6 +1275,10 @@ void UnwrappedLineParser::parsePPDefine() {
>>>      addUnwrappedLine();
>>>      ++Line->Level;
>>>      Line->InMacroBody = true;
>>>   +  if (Style.IndentPPDirectives != FormatStyle::PPDIS_None) {
>>>   +    Line->PPLevel =
>>>   +        IncludeGuard == IG_Defined ? PPBranchLevel : PPBranchLevel + 1;
>>>   +  }
>>>    
>>>      // Errors during a preprocessor directive can only affect the layout of the
>>>      // preprocessor directive, and thus we ignore them. An alternative approach
>>
>> You're right this works (but off by one). Updated patch.
>
> It was not off by 1, or else it wouldn't have worked. Below is how I defined `PPLevel`:
>
>   $ git diff UnwrappedLineParser.h
>   diff --git a/clang/lib/Format/UnwrappedLineParser.h b/clang/lib/Format/UnwrappedLineParser.h
>   index b9b106bcc89a..a234f6852e0c 100644
>   --- a/clang/lib/Format/UnwrappedLineParser.h
>   +++ b/clang/lib/Format/UnwrappedLineParser.h
>   @@ -43,6 +43,9 @@ struct UnwrappedLine {
>    
>      /// The indent level of the \c UnwrappedLine.
>      unsigned Level;
>   +  /// The \c PPBranchLevel (adjusted for header guards) of the macro definition
>   +  /// this line belongs to.
>   +  unsigned PPLevel;
>    
>      /// Whether this \c UnwrappedLine is part of a preprocessor directive.
>      bool InPPDirective;
>   @@ -358,7 +361,7 @@ struct UnwrappedLineNode {
>    };
>    
>    inline UnwrappedLine::UnwrappedLine()
>   -    : Level(0), InPPDirective(false), InPragmaDirective(false),
>   +    : Level(0), PPLevel(0), InPPDirective(false), InPragmaDirective(false),
>          InMacroBody(false), MustBeDeclaration(false),
>          MatchingOpeningBlockLineIndex(kInvalidIndex) {}
>    
>
> Conceptually, I think it's more accurate to make `PPLevel` to mean the PP branching level of the `#define` line, not the first line of the macro body. IMO it may simplify the changes you made to the formatter.

Hmm? Not sure what you mean.

> BTW you still need to change `PPLevel` to `unsigned` and initialize it as requested in D137181#inline-1328385 <https://reviews.llvm.org/D137181#inline-1328385>.

Fixed.



================
Comment at: clang/lib/Format/UnwrappedLineParser.h:66
+  /// #endif                PPLevel still at : 0
+  int PPLevel;
+
----------------
owenpan wrote:
> goldstein.w.n wrote:
> > owenpan wrote:
> > > You need to initialize `PPLevel` in the constructor. FWIW I'd use `unsigned` to be consistent with `Level` above.
> > Will initialize. It's `int` because we do a less than zero check in the Formatter.
> > It's `int` because we do a less than zero check in the Formatter.
> 
> If you set `PPLevel` to `PPBranchLevel` when there is no header guard, and to `PPBranchLevel + 1`otherwise, `PPLevel` can never be negative.
Sorry didnt see your reply. Fixed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137181