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