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

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.


================
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;
----------------
euhlmann wrote:
> djasper wrote:
> > 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).
> I apologize, the style option documentation was a typo. The patch summary has the intended behavior, and I mentioned there that I count the hash as a column. Part of the reasoning for this is because it would look the same visually with spaces or tabs.
Do you know of a coding style that writes something about this? I think the similarity of spaces vs. tabs is not a strong reason because a source file will either use one or the other. To me:

  #if a == 1
  # define X
  # if b == 2
  #   define Y
  ...

Would look weird. I'd prefer if we kept this simpler and more consistent.


================
Comment at: lib/Format/UnwrappedLineParser.cpp:701
+      PPMaybeIncludeGuard->TokenText == FormatTok->TokenText &&
+      PPIndentLevel > 0) {
+    --PPIndentLevel;
----------------
euhlmann wrote:
> djasper wrote:
> > 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
> It sounds like these would require two passes, since we wouldn't know until the end whether an include guard spans the whole file or has a #else. Do we really want two passes? Or do you have any advice on how these tests can be implemented?
You could store an additional bool that gets set to true if the possible include guard is matched up with an appropriate #endif in the last line of a file. If that bool is true, you can remove 1 from the level of all preprocessor lines.


https://reviews.llvm.org/D35955