[PATCH] D35955: clang-format: Add preprocessor directive indentation

[Krasimir Georgiev via Phabricator via cfe-commits]

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