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

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.


================
Comment at: docs/ClangFormatStyleOptions.rst:1182
 
+**IndentPPDirectives** (``bool``)
+  Indent preprocessor directives on conditionals.
----------------
I think we can foresee that a bool is not going to be enough here. Make this an enum, which for no can contain the values "no" and "afterhash" and in the long run can get a "beforehash" value.


================
Comment at: lib/Format/ContinuationIndenter.cpp:379
 
+  // indent preprocessor directives after the hash if required.
+  if ((State.Line->Type == LT_PreprocessorDirective ||
----------------
Start comments upper case.


================
Comment at: lib/Format/ContinuationIndenter.cpp:383
+      Current.Previous->is(tok::hash) && State.FirstIndent > 0) {
+    // subtract 1 so indent lines up with non-preprocessor code
+    Spaces += State.FirstIndent;
----------------
Same here and use full sentences.

Also, this does not seem to be what the example in the style option suggests and I'd rather not do it (subtracting 1).


================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:1021
 
+  // Preprocessor directives get indented after the hash.
+  if (Line.Type == LT_PreprocessorDirective ||
----------------
... or not at all. :)


================
Comment at: lib/Format/UnwrappedLineParser.cpp:667
   parsePPUnknown();
+  if (IfNDef) {
+    PPMaybeIncludeGuard = IfCondition;
----------------
LLVM style does not use braces for single statement ifs.


================
Comment at: lib/Format/UnwrappedLineParser.cpp:699
   }
+  if (PPMaybeIncludeGuard != nullptr &&
+      PPMaybeIncludeGuard->TokenText == FormatTok->TokenText &&
----------------
In LLVM's codebase, we mostly just leave out the "!= nullptr" and rely on the implicit conversion to bool.


================
Comment at: lib/Format/UnwrappedLineParser.cpp:701
+      PPMaybeIncludeGuard->TokenText == FormatTok->TokenText &&
+      PPIndentLevel > 0) {
+    --PPIndentLevel;
----------------
I think you'll need substantially more tests here:
- An include guard must not have a #else 
- An include guard must span the entire file


================
Comment at: lib/Format/UnwrappedLineParser.h:238
 
+  unsigned PPIndentLevel;
+  FormatToken *PPMaybeIncludeGuard;
----------------
I think this should almost always be PPBranchLevel. Probably the different case is the #else itself, but I think we can handle that easily.


https://reviews.llvm.org/D35955