krasimir added inline comments.
================
Comment at: lib/Format/ContinuationIndenter.cpp:383
+ State.Line->Type == LT_ImportStatement) &&
+ Current.Previous->is(tok::hash) && State.FirstIndent > 0) {
+ Spaces += State.FirstIndent;
----------------
You can replace `Current.Previous` with `Previous`. Also, I'd swap the checks a
bit, like:
```
if (Previous.is(tok::hash) && State.FirstIndent > 0 &&
Style.IndentPPDirectives == FormatStyle::PPDIS_AfterHash &&
(State.Line->Type == LT_PreprocessorDirective ||
State.Line->Type == LT_ImportStatement)) {
```
That way, the common case `Previous.is(tok::hash) == false` is handled quickly.
================
Comment at: lib/Format/ContinuationIndenter.cpp:387
+ // hash. This causes second-level indents onward to have an extra space
+ // after the tabs. We set the state to column 0 to avoid this misalignment.
+ if (Style.UseTab != FormatStyle::UT_Never)
----------------
I don't understand this comment. Could you please give an example?
================
Comment at: lib/Format/UnwrappedLineParser.cpp:701
+ bool MaybeIncludeGuard = IfNDef;
+ for (auto& Line : Lines) {
+ if (!Line.Tokens.front().Tok->is(tok::comment)) {
----------------
This can easily lead to a pretty bad runtime: consider a file starting with a
few hundred lines of comments and having a few hundred `#ifdef`-s.
I'd say that after we do this MaybeIncludeGuard thing once, we don't repeat it
again.
Also, lines could start with a block comment and continue with code:
```
/* small */ int ten = 10;
```
================
Comment at: lib/Format/UnwrappedLineParser.cpp:732
+ // then we count it as a real include guard and subtract one from every
+ // preprocessor indent.
+ unsigned TokenPosition = Tokens->getPosition();
----------------
Why do we need to subtract one from every preprocessor indent?
================
Comment at: lib/Format/UnwrappedLineParser.cpp:737
+ for (auto& Line : Lines) {
+ if (Line.InPPDirective && Line.Level > 0)
+ --Line.Level;
----------------
Wouldn't this also indent lines continuing macro definitions, as in:
```
#define A(x) int f(int x) { \
return x; \
}
```
================
Comment at: lib/Format/UnwrappedLineParser.cpp:760
+ }
+ PPMaybeIncludeGuard = nullptr;
nextToken();
----------------
Why do we reset `PPMaybeIncludeGuard` here?
================
Comment at: lib/Format/UnwrappedLineParser.cpp:770
addUnwrappedLine();
- Line->Level = 1;
+ ++Line->Level;
----------------
Why do we `++Line->Level` here?
================
Comment at: lib/Format/UnwrappedLineParser.cpp:787
addUnwrappedLine();
+ PPMaybeIncludeGuard = nullptr;
}
----------------
Why do we reset `PPMaybeIncludeGuard` here?
================
Comment at: lib/Format/UnwrappedLineParser.h:249
+ FormatToken *PPMaybeIncludeGuard;
+ bool FoundIncludeGuardStart;
----------------
Please add a comment for `PPMaybeIncludeGuard`: is it expected to point to the
hash token of the `#ifdef`, or?
https://reviews.llvm.org/D35955
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
