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

[Krasimir Georgiev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL312125: clang-format: Add preprocessor directive indentation 
(authored by krasimir).

Changed prior to commit:
  https://reviews.llvm.org/D35955?vs=112432=113261#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D35955

Files:
  cfe/trunk/docs/ClangFormatStyleOptions.rst
  cfe/trunk/include/clang/Format/Format.h
  cfe/trunk/lib/Format/ContinuationIndenter.cpp
  cfe/trunk/lib/Format/Format.cpp
  cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp
  cfe/trunk/lib/Format/UnwrappedLineParser.cpp
  cfe/trunk/lib/Format/UnwrappedLineParser.h
  cfe/trunk/unittests/Format/FormatTest.cpp
  cfe/trunk/unittests/Format/FormatTestUtils.h

Index: cfe/trunk/lib/Format/UnwrappedLineParser.cpp
===
--- cfe/trunk/lib/Format/UnwrappedLineParser.cpp
+++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp
@@ -231,10 +231,15 @@
 : Line(new UnwrappedLine), MustBreakBeforeNextToken(false),
   CurrentLines(), Style(Style), Keywords(Keywords),
   CommentPragmasRegex(Style.CommentPragmas), Tokens(nullptr),
-  Callback(Callback), AllTokens(Tokens), PPBranchLevel(-1) {}
+  Callback(Callback), AllTokens(Tokens), PPBranchLevel(-1),
+  IfNdefCondition(nullptr), FoundIncludeGuardStart(false),
+  IncludeGuardRejected(false) {}
 
 void UnwrappedLineParser::reset() {
   PPBranchLevel = -1;
+  IfNdefCondition = nullptr;
+  FoundIncludeGuardStart = false;
+  IncludeGuardRejected = false;
   Line.reset(new UnwrappedLine);
   CommentsBeforeNextToken.clear();
   FormatTok = nullptr;
@@ -679,7 +684,7 @@
 }
   }
   // Guard against #endif's without #if.
-  if (PPBranchLevel > 0)
+  if (PPBranchLevel > -1)
 --PPBranchLevel;
   if (!PPChainBranchIndex.empty())
 PPChainBranchIndex.pop();
@@ -696,19 +701,53 @@
   if (IfDef && !IfNDef && FormatTok->TokenText == "SWIG")
 Unreachable = true;
   conditionalCompilationStart(Unreachable);
+  FormatToken *IfCondition = FormatTok;
+  // If there's a #ifndef on the first line, and the only lines before it are
+  // comments, it could be an include guard.
+  bool MaybeIncludeGuard = IfNDef;
+  if (!IncludeGuardRejected && !FoundIncludeGuardStart && MaybeIncludeGuard) {
+for (auto  : Lines) {
+  if (!Line.Tokens.front().Tok->is(tok::comment)) {
+MaybeIncludeGuard = false;
+IncludeGuardRejected = true;
+break;
+  }
+}
+  }
+  --PPBranchLevel;
   parsePPUnknown();
+  ++PPBranchLevel;
+  if (!IncludeGuardRejected && !FoundIncludeGuardStart && MaybeIncludeGuard)
+IfNdefCondition = IfCondition;
 }
 
 void UnwrappedLineParser::parsePPElse() {
+  // If a potential include guard has an #else, it's not an include guard.
+  if (FoundIncludeGuardStart && PPBranchLevel == 0)
+FoundIncludeGuardStart = false;
   conditionalCompilationAlternative();
+  if (PPBranchLevel > -1)
+--PPBranchLevel;
   parsePPUnknown();
+  ++PPBranchLevel;
 }
 
 void UnwrappedLineParser::parsePPElIf() { parsePPElse(); }
 
 void UnwrappedLineParser::parsePPEndIf() {
   conditionalCompilationEnd();
   parsePPUnknown();
+  // If the #endif of a potential include guard is the last thing in the file,
+  // then we count it as a real include guard and subtract one from every
+  // preprocessor indent.
+  unsigned TokenPosition = Tokens->getPosition();
+  FormatToken *PeekNext = AllTokens[TokenPosition];
+  if (FoundIncludeGuardStart && PPBranchLevel == -1 && PeekNext->is(tok::eof)) {
+for (auto  : Lines) {
+  if (Line.InPPDirective && Line.Level > 0)
+--Line.Level;
+}
+  }
 }
 
 void UnwrappedLineParser::parsePPDefine() {
@@ -718,14 +757,26 @@
 parsePPUnknown();
 return;
   }
+  if (IfNdefCondition && IfNdefCondition->TokenText == FormatTok->TokenText) {
+FoundIncludeGuardStart = true;
+for (auto  : Lines) {
+  if (!Line.Tokens.front().Tok->isOneOf(tok::comment, tok::hash)) {
+FoundIncludeGuardStart = false;
+break;
+  }
+}
+  }
+  IfNdefCondition = nullptr;
   nextToken();
   if (FormatTok->Tok.getKind() == tok::l_paren &&
   FormatTok->WhitespaceRange.getBegin() ==
   FormatTok->WhitespaceRange.getEnd()) {
 parseParens();
   }
+  if (Style.IndentPPDirectives == FormatStyle::PPDIS_AfterHash)
+Line->Level += PPBranchLevel + 1;
   addUnwrappedLine();
-  Line->Level = 1;
+  ++Line->Level;
 
   // Errors during a preprocessor directive can only affect the layout of the
   // preprocessor directive, and thus we ignore them. An alternative approach
@@ -739,7 +790,10 @@
   do {
 nextToken();
   } while (!eof());
+  if (Style.IndentPPDirectives == FormatStyle::PPDIS_AfterHash)
+Line->Level += PPBranchLevel + 1;
   addUnwrappedLine();
+  IfNdefCondition = nullptr;
 }
 
 // Here we blacklist certain tokens that are not usually the first token in an
Index: cfe/trunk/lib/Format/Format.cpp

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

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added a comment.

I'll land this for you, as discussed offline. The best thing is to apply for 
commit rights 
 after you 
have a few patches landed.


https://reviews.llvm.org/D35955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added a comment.

@euhlmann: are you planning to commit this?


https://reviews.llvm.org/D35955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erik Uhlmann via Phabricator via cfe-commits]

euhlmann added a comment.

I'm glad this is finally in a state to land. Thanks for the helpful reviews!


https://reviews.llvm.org/D35955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added a comment.

Thank you! I understand this patch better now. Looks good from my side!


https://reviews.llvm.org/D35955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

From my side this looks good for now (we can always improve more later). 
Krasimir, what do you think?


https://reviews.llvm.org/D35955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mark Zeren via Phabricator via cfe-commits]

mzeren-vmw 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;

djasper wrote:
> 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.
@djasper I just noticed that the [[ 
https://google.github.io/styleguide/cppguide.html#Preprocessor_Directives | 
Google Style Guide  ]] has:
```
#if DISASTER_PENDING  // Correct -- Starts at beginning of line
DropEverything();
# if NOTIFY   // OK but not required -- Spaces after #
NotifyClient();
# endif
#endif
```
Note the single space in `# if NOTIFY`. Can we correct the Guide to match what 
we have here?



https://reviews.llvm.org/D35955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erik Uhlmann via Phabricator via cfe-commits]

euhlmann added inline comments.



Comment at: lib/Format/UnwrappedLineParser.cpp:737
+for (auto& Line : Lines) {
+  if (Line.InPPDirective && Line.Level > 0)
+--Line.Level;

krasimir wrote:
> Wouldn't this also indent lines continuing macro definitions, as in:
> ```
> #define A(x) int f(int x) { \
>   return x; \
> }
> ```
Multi-line macros look this under this patch

```
#if A
#  define MACRO\
void a(int x) {\
  b(); \
  c(); \
  d(); \
  e(); \
  f(); \
}
#endif
```

With previous clang-format, no preprocessor indentation:


```
#if A
#define MACRO  \
  void a(int x) {  \
b();   \
c();   \
d();   \
e();   \
f();   \
  }
#endif
```


https://reviews.llvm.org/D35955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erik Uhlmann via Phabricator via cfe-commits]

euhlmann updated this revision to Diff 112432.
euhlmann marked 7 inline comments as done.
euhlmann added a comment.

- Swapped checks in ContinuationIndenter
- Renamed `PPMaybeIncludeGuard` to `IfNdefCondition` and added comment
- Added another bool member that tracks if include guard detection was rejected 
once. If the bool gets set, the loop checking previous lines doesn't run again.
- Set lower ColumnLimit in tests
- Fixed incorrect column for `\` in multi-line macros
- Resolved comment in FormatTestUtils


https://reviews.llvm.org/D35955

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  lib/Format/UnwrappedLineFormatter.cpp
  lib/Format/UnwrappedLineParser.cpp
  lib/Format/UnwrappedLineParser.h
  unittests/Format/FormatTest.cpp
  unittests/Format/FormatTestUtils.h

Index: unittests/Format/FormatTestUtils.h
===
--- unittests/Format/FormatTestUtils.h
+++ unittests/Format/FormatTestUtils.h
@@ -30,7 +30,8 @@
   if (JustReplacedNewline)
 MessedUp[i - 1] = '\n';
   InComment = true;
-} else if (MessedUp[i] == '#' && (JustReplacedNewline || i == 0)) {
+} else if (MessedUp[i] == '#' &&
+   (JustReplacedNewline || i == 0 || MessedUp[i - 1] == '\n')) {
   if (i != 0)
 MessedUp[i - 1] = '\n';
   InPreprocessorDirective = true;
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2286,8 +2286,183 @@
getLLVMStyleWithColumns(11));
 }
 
-TEST_F(FormatTest, IndentPreprocessorDirectivesAtZero) {
-  EXPECT_EQ("{\n  {\n#define A\n  }\n}", format("{{\n#define A\n}}"));
+TEST_F(FormatTest, IndentPreprocessorDirectives) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentPPDirectives = FormatStyle::PPDIS_None;
+  Style.ColumnLimit = 40;
+  verifyFormat("#ifdef _WIN32\n"
+   "#define A 0\n"
+   "#ifdef VAR2\n"
+   "#define B 1\n"
+   "#include \n"
+   "#define MACRO  \\\n"
+   "  some_very_long_func_aa();\n"
+   "#endif\n"
+   "#else\n"
+   "#define A 1\n"
+   "#endif",
+   Style);
+
+  Style.IndentPPDirectives = FormatStyle::PPDIS_AfterHash;
+  verifyFormat("#ifdef _WIN32\n"
+   "#  define A 0\n"
+   "#  ifdef VAR2\n"
+   "#define B 1\n"
+   "#include \n"
+   "#define MACRO  \\\n"
+   "  some_very_long_func_aa();\n"
+   "#  endif\n"
+   "#else\n"
+   "#  define A 1\n"
+   "#endif",
+   Style);
+  verifyFormat("#if A\n"
+   "#  define MACRO\\\n"
+   "void a(int x) {\\\n"
+   "  b(); \\\n"
+   "  c(); \\\n"
+   "  d(); \\\n"
+   "  e(); \\\n"
+   "  f(); \\\n"
+   "}\n"
+   "#endif",
+   Style);
+  // Comments before include guard.
+  verifyFormat("// file comment\n"
+   "// file comment\n"
+   "#ifndef HEADER_H\n"
+   "#define HEADER_H\n"
+   "code();\n"
+   "#endif",
+   Style);
+  // Test with include guards.
+  // EXPECT_EQ is used because verifyFormat() calls messUp() which incorrectly
+  // merges lines.
+  verifyFormat("#ifndef HEADER_H\n"
+   "#define HEADER_H\n"
+   "code();\n"
+   "#endif",
+   Style);
+  // Include guards must have a #define with the same variable immediately
+  // after #ifndef.
+  verifyFormat("#ifndef NOT_GUARD\n"
+   "#  define FOO\n"
+   "code();\n"
+   "#endif",
+   Style);
+
+  // Include guards must cover the entire file.
+  verifyFormat("code();\n"
+   "code();\n"
+   "#ifndef NOT_GUARD\n"
+   "#  define NOT_GUARD\n"
+   "code();\n"
+   "#endif",
+   Style);
+  verifyFormat("#ifndef NOT_GUARD\n"
+   "#  define NOT_GUARD\n"
+   "code();\n"
+   "#endif\n"
+   "code();",
+   Style);
+  // Test with trailing blank lines.
+  verifyFormat("#ifndef HEADER_H\n"
+   "#define HEADER_H\n"
+   "code();\n"
+   "#endif\n",
+   Style);
+  // Include guards don't have #else.
+  verifyFormat("#ifndef NOT_GUARD\n"
+   "#  define NOT_GUARD\n"
+   

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

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: unittests/Format/FormatTest.cpp:2297
+   "#include \n"
+   "#define MACRO  
"
+   "\\\n"

Please just set Style.ColumnLimit to 60 or so in order not to have to wrap the 
test cases. Makes then much easier to understand.



Comment at: unittests/Format/FormatTest.cpp:2314
+   "#define MACRO  
"
+   "\\\n"
+   "  
some_very_long_func_a"

It's hard to tell with the line wrap, but this does look wrong, the escaped 
newline does not seem to be in column 80.



Comment at: unittests/Format/FormatTest.cpp:2427
+   Style));
+  // Defect: The comment indent corrector in TokenAnnotator gets thrown off by
+  // preprocessor indentation.

Write "FIXME" instead of "Defect"



Comment at: unittests/Format/FormatTestUtils.h:34
   InComment = true;
-} else if (MessedUp[i] == '#' && (JustReplacedNewline || i == 0)) {
+} else if (MessedUp[i] == '#' && (JustPassedNewline || i == 0)) {
   if (i != 0)

Could we just do:

  } else if (MessedUp[i] == '#' && (JustReplacedNewline || i == 0 || MessedUp[i 
- 1] == '\n') {

?


https://reviews.llvm.org/D35955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erik Uhlmann via Phabricator via cfe-commits]

euhlmann added inline comments.



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)

krasimir wrote:
> I don't understand this comment. Could you please give an example?
When tabs are enabled, extra spaces would be added because of the column offset 
caused by the `#` token.
```
#if FOO
#if BAR
#define BAZ
#endif
#endif
```
Let me know if there's a better way to fix that.



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();

krasimir wrote:
> Why do we need to subtract one from every preprocessor indent?
This only knows for sure when something is an include guard at the end of the 
file. By that time, everything inside the include guard has already been 
indented one extra level since we didn't know about the guard yet, so it would 
look like
```
#ifndef FILE_H
#  define FILE_H
// ...
#  other directives
// ...
#endif
```
Subtracting one is the action taken when we decide that there's a valid include 
guard, so it looks like
```
#ifndef FILE_H
#define FILE_H
// ...
#other directives
// ...
#endif
```



Comment at: lib/Format/UnwrappedLineParser.cpp:760
+  }
+  PPMaybeIncludeGuard = nullptr;
   nextToken();

krasimir wrote:
> Why do we reset `PPMaybeIncludeGuard` here?
The idea is if there's any preprocessor lines between the #ifndef and the 
#define, it doesn't treat it as an include guard. This is the same reason for 
the other PPMaybeIncludeGuard reset.



Comment at: lib/Format/UnwrappedLineParser.cpp:770
   addUnwrappedLine();
-  Line->Level = 1;
+  ++Line->Level;
 

krasimir wrote:
> Why do we `++Line->Level` here?
This sets correct indent for multi-line macros, so they are indented one level 
more than the indent of the #define instead of always at level 1.


https://reviews.llvm.org/D35955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

Krasimir: Can you actually give this a round of review? I will also try to do 
so tomorrow.


https://reviews.llvm.org/D35955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erik Uhlmann via Phabricator via cfe-commits]

euhlmann updated this revision to Diff 111606.
euhlmann marked an inline comment as done.
euhlmann edited the summary of this revision.
euhlmann added a comment.

Eliminated redundancy in tests by fixing test::messUp which was incorrectly 
merging lines.

Fixing the issue Mark brought up regarding comments before indented 
preprocessor lines appears to be a major change and I believe it's too much 
complexity for this patch.


https://reviews.llvm.org/D35955

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  lib/Format/TokenAnnotator.cpp
  lib/Format/UnwrappedLineFormatter.cpp
  lib/Format/UnwrappedLineParser.cpp
  lib/Format/UnwrappedLineParser.h
  unittests/Format/FormatTest.cpp
  unittests/Format/FormatTestUtils.h

Index: unittests/Format/FormatTestUtils.h
===
--- unittests/Format/FormatTestUtils.h
+++ unittests/Format/FormatTestUtils.h
@@ -25,19 +25,21 @@
   bool InComment = false;
   bool InPreprocessorDirective = false;
   bool JustReplacedNewline = false;
+  bool JustPassedNewline = false;
   for (unsigned i = 0, e = MessedUp.size() - 1; i != e; ++i) {
 if (MessedUp[i] == '/' && MessedUp[i + 1] == '/') {
   if (JustReplacedNewline)
 MessedUp[i - 1] = '\n';
   InComment = true;
-} else if (MessedUp[i] == '#' && (JustReplacedNewline || i == 0)) {
+} else if (MessedUp[i] == '#' && (JustPassedNewline || i == 0)) {
   if (i != 0)
 MessedUp[i - 1] = '\n';
   InPreprocessorDirective = true;
 } else if (MessedUp[i] == '\\' && MessedUp[i + 1] == '\n') {
   MessedUp[i] = ' ';
   MessedUp[i + 1] = ' ';
 } else if (MessedUp[i] == '\n') {
+  JustPassedNewline = true;
   if (InComment) {
 InComment = false;
   } else if (InPreprocessorDirective) {
@@ -48,6 +50,7 @@
   }
 } else if (MessedUp[i] != ' ') {
   JustReplacedNewline = false;
+  JustPassedNewline = false;
 }
   }
   std::string WithoutWhitespace;
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2286,8 +2286,176 @@
getLLVMStyleWithColumns(11));
 }
 
-TEST_F(FormatTest, IndentPreprocessorDirectivesAtZero) {
-  EXPECT_EQ("{\n  {\n#define A\n  }\n}", format("{{\n#define A\n}}"));
+TEST_F(FormatTest, IndentPreprocessorDirectives) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentPPDirectives = FormatStyle::PPDIS_None;
+  verifyFormat("#ifdef _WIN32\n"
+   "#define A 0\n"
+   "#ifdef VAR2\n"
+   "#define B 1\n"
+   "#include \n"
+   "#define MACRO  "
+   "\\\n"
+   "  some_very_long_func_a"
+   "a();\n"
+   "#endif\n"
+   "#else\n"
+   "#define A 1\n"
+   "#endif",
+   Style);
+
+  Style.IndentPPDirectives = FormatStyle::PPDIS_AfterHash;
+  verifyFormat("#ifdef _WIN32\n"
+   "#  define A 0\n"
+   "#  ifdef VAR2\n"
+   "#define B 1\n"
+   "#include \n"
+   "#define MACRO  "
+   "\\\n"
+   "  some_very_long_func_a"
+   "a();\n"
+   "#  endif\n"
+   "#else\n"
+   "#  define A 1\n"
+   "#endif",
+   Style);
+  // Comments before include guard.
+  verifyFormat("// file comment\n"
+   "// file comment\n"
+   "#ifndef HEADER_H\n"
+   "#define HEADER_H\n"
+   "code();\n"
+   "#endif",
+   Style);
+  // Test with include guards.
+  // EXPECT_EQ is used because verifyFormat() calls messUp() which incorrectly
+  // merges lines.
+  verifyFormat("#ifndef HEADER_H\n"
+   "#define HEADER_H\n"
+   "code();\n"
+   "#endif",
+   Style);
+  // Include guards must have a #define with the same variable immediately
+  // after #ifndef.
+  verifyFormat("#ifndef NOT_GUARD\n"
+   "#  define FOO\n"
+   "code();\n"
+   "#endif",
+   Style);
+
+  // Include guards must cover the entire file.
+  verifyFormat("code();\n"
+   "code();\n"
+   "#ifndef NOT_GUARD\n"
+   "#  define NOT_GUARD\n"
+   "code();\n"
+   "#endif",
+   Style);
+  verifyFormat("#ifndef NOT_GUARD\n"
+   "#  define NOT_GUARD\n"
+   "code();\n"
+   "#endif\n"
+   "code();",
+   Style);

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

[Erik Uhlmann via Phabricator via cfe-commits]

euhlmann updated this revision to Diff 111433.
euhlmann marked 13 inline comments as done.
euhlmann edited the summary of this revision.
euhlmann added a comment.

Allows comments before the include guard opens. However, if there's a single 
non-comment line before an include-guard-like structure that meets all the 
other conditions then it will be treated as an include guard because `Lines` 
does not yet contain the line before the `#ifndef`. This looks difficult to fix 
and I believe it's a rare enough situation to ignore.

This does not yet address avoiding repetition in the tests or comments before 
indented preprocessor lines being indented.


https://reviews.llvm.org/D35955

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  lib/Format/TokenAnnotator.cpp
  lib/Format/UnwrappedLineFormatter.cpp
  lib/Format/UnwrappedLineParser.cpp
  lib/Format/UnwrappedLineParser.h
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2286,8 +2286,206 @@
getLLVMStyleWithColumns(11));
 }
 
-TEST_F(FormatTest, IndentPreprocessorDirectivesAtZero) {
-  EXPECT_EQ("{\n  {\n#define A\n  }\n}", format("{{\n#define A\n}}"));
+TEST_F(FormatTest, FooBar) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentPPDirectives = FormatStyle::PPDIS_AfterHash;
+  llvm::dbgs() <<
+format("code();\n"
+   "#ifndef _SOMEFILE_H\n"
+   "#define _SOMEFILE_H\n"
+   "code();\n"
+   "#endif",
+   Style) << "\n";
+}
+
+TEST_F(FormatTest, IndentPreprocessorDirectives) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentPPDirectives = FormatStyle::PPDIS_None;
+  verifyFormat("#ifdef _WIN32\n"
+   "#define A 0\n"
+   "#ifdef VAR2\n"
+   "#define B 1\n"
+   "#include \n"
+   "#define MACRO  "
+   "\\\n"
+   "  some_very_long_func_a"
+   "a();\n"
+   "#endif\n"
+   "#else\n"
+   "#define A 1\n"
+   "#endif",
+   Style);
+
+  Style.IndentPPDirectives = FormatStyle::PPDIS_AfterHash;
+  verifyFormat("#ifdef _WIN32\n"
+   "#  define A 0\n"
+   "#  ifdef VAR2\n"
+   "#define B 1\n"
+   "#include \n"
+   "#define MACRO  "
+   "\\\n"
+   "  some_very_long_func_a"
+   "a();\n"
+   "#  endif\n"
+   "#else\n"
+   "#  define A 1\n"
+   "#endif",
+   Style);
+  // Comments before include guard.
+  verifyFormat("// file comment\n"
+   "// file comment\n"
+   "#ifndef HEADER_H\n"
+   "#define HEADER_H\n"
+   "code();\n"
+   "#endif",
+   Style);
+  // Test with include guards.
+  // EXPECT_EQ is used because verifyFormat() calls messUp() which incorrectly
+  // merges lines.
+  EXPECT_EQ("#ifndef HEADER_H\n"
+"#define HEADER_H\n"
+"code();\n"
+"#endif",
+format("#ifndef HEADER_H\n"
+   "#define HEADER_H\n"
+   "code();\n"
+   "#endif",
+   Style));
+  // Include guards must have a #define with the same variable immediately
+  // after #ifndef.
+  EXPECT_EQ("#ifndef NOT_GUARD\n"
+"#  define FOO\n"
+"code();\n"
+"#endif",
+format("#ifndef NOT_GUARD\n"
+   "#  define FOO\n"
+   "code();\n"
+   "#endif",
+   Style));
+
+  // Include guards must cover the entire file.
+  EXPECT_EQ("code();\n"
+"code();\n"
+"#ifndef NOT_GUARD\n"
+"#  define NOT_GUARD\n"
+"code();\n"
+"#endif",
+format("code();\n"
+   "code();\n"
+   "#ifndef NOT_GUARD\n"
+   "#  define NOT_GUARD\n"
+   "code();\n"
+   "#endif",
+   Style));
+  EXPECT_EQ("#ifndef NOT_GUARD\n"
+"#  define NOT_GUARD\n"
+"code();\n"
+"#endif\n"
+"code();",
+format("#ifndef NOT_GUARD\n"
+   "#  define NOT_GUARD\n"
+   "code();\n"
+   "#endif\n"
+   "code();",
+   Style));
+  // Test with trailing blank lines.
+  EXPECT_EQ("#ifndef 

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

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: unittests/Format/FormatTest.cpp:2281
 
 TEST_F(FormatTest, LayoutMacroDefinitionsStatementsSpanningBlocks) {
   verifyFormat("#define A \\\n"

mzeren-vmw wrote:
> mzeren-vmw wrote:
> > Experimenting with the patch locally I found that comment indentation is 
> > broken in some cases. Please add tests that cover comments. For example:
> > 
> > comment indented at code level as expected:
> >   void f() {
> >   #if 0
> > // Comment
> > code();
> >   #  define FOO 0
> >   #endif
> >   }
> > comment not indented at code level when there's a guard:
> >   #ifndef _SOMEFILE_H
> >   #define _SOMEFILE_H
> >   void f() {
> >   #if 0
> >   // Comment
> > code();
> >   #  define FOO 0
> >   #endif
> >   }
> >   #endif
> > 
> > The `#define FOO 0` is required to trigger this behavior.
> Erik spent some time investigating issues with comment indentation. A couple 
> of insights so far:
> 
> a) We need tests for wrapped macros (with trailing \ continuations). These 
> need to include embedded comments.
> 
> b) It would be most natural to align comments with the following non-comment 
> line. So a comment may be indented at "preprocessor level" or "code level". 
> If indented at preprocessor level we have to handle the extra space 
> introduced by the leading hash. This may require an extra bit per line to 
> indicate whether the "aligned-to" line is preprocessor or code.
"#if 0" specifically might not be a good candidate for a test case as that 
should be treated like a comment. We should not try to format anything inside a 
"#if 0" and I'd be surprised if clang-format touched that at all. "#if A" or 
something is more appropriate.



Comment at: unittests/Format/FormatTest.cpp:2322-2331
+  // Test with include guards.
+  EXPECT_EQ("#ifndef _SOMEFILE_H\n"
+"#define _SOMEFILE_H\n"
+"code();\n"
+"#endif",
+format("#ifndef _SOMEFILE_H\n"
+   "#define _SOMEFILE_H\n"

mzeren-vmw wrote:
> A DRY-er way to say this is:
>   auto WithGuard = "#ifndef _SOMEFILE_H\n"
>"#define _SOMEFILE_H\n"
>"code();\n"
>"#endif";
>   ASSERT_EQ(WithGuards, format(WithGuards, Style));
> 
I think _A... is actually not an identifier that is ok to use. Remove the 
leading "_".



Comment at: unittests/Format/FormatTest.cpp:2334
+  // after #ifndef.
+  EXPECT_EQ("#ifndef NOT_GUARD\n"
+"#  define FOO\n"

mzeren-vmw wrote:
> 
> It would be nice to have a comment that explains why you cannot use 
> verifyFormat.
Or even have a separate function that messes this up in some other way, not 
altering line breaks so that we can keep these tests short.



Comment at: unittests/Format/FormatTest.cpp:2405-2408
+  // Defect: We currently do not deal with cases where legitimate lines may be
+  // outside an include guard. Examples are #pragma once and
+  // #pragma GCC diagnostic, or anything else that does not change the meaning
+  // of the file if it's included multiple times.

euhlmann wrote:
> mzeren-vmw wrote:
> > We need to handle comments (like copyrights) before the include guard. 
> > There is an analogous problem with trailing blank lines, or trailing 
> > comments.
> > 
> > I think we need a small state machine: Init, Start, Open, Closed, NotGuard 
> > (terminal). `FoundIncludeGuardStart` should change from a bool to an enum 
> > to track this state. `PPMaybeIncludeGuard` can then drop it's "mabye". 
> > Whether or not it is null depends directly on the state. It does not 
> > determine state itself. While parsing, each line updates the state. If we 
> > get to `eof` in the Closed state then we have detected an include guard. 
> > ... Or similar logic
> > 
> > Note that support for comments before the guard opens the door to support 
> > for #pragma once. It is tempting to add that, but that is a slippery slope. 
> > I would recommend leaving that as a defect that can be addressed later.
> @djasper @klimek
> Do you have any comments on this? I've begun to implement an enum/state 
> machine but I'd like to know if you'd like me to implement a different way.
Most of the work here is done in the UnwrappedLineParser, which should skip 
over comments anyway. So yes, we should allow for these comments, but I don't 
think the logic needs to get much more complex because of that. I strongly 
doubt that a state machine is the right mechanism here.

And generally (without specific action to derive from that), I think we should 
try to err on the side of incorrectly finding header guards instead of 
incorrectly not recognizing a header guard. I think a not-indented #define 
somewhere is better than an incorrectly indented header guard.


https://reviews.llvm.org/D35955



___
cfe-commits mailing list

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

[Erik Uhlmann via Phabricator via cfe-commits]

euhlmann added inline comments.



Comment at: unittests/Format/FormatTest.cpp:2405-2408
+  // Defect: We currently do not deal with cases where legitimate lines may be
+  // outside an include guard. Examples are #pragma once and
+  // #pragma GCC diagnostic, or anything else that does not change the meaning
+  // of the file if it's included multiple times.

mzeren-vmw wrote:
> We need to handle comments (like copyrights) before the include guard. There 
> is an analogous problem with trailing blank lines, or trailing comments.
> 
> I think we need a small state machine: Init, Start, Open, Closed, NotGuard 
> (terminal). `FoundIncludeGuardStart` should change from a bool to an enum to 
> track this state. `PPMaybeIncludeGuard` can then drop it's "mabye". Whether 
> or not it is null depends directly on the state. It does not determine state 
> itself. While parsing, each line updates the state. If we get to `eof` in the 
> Closed state then we have detected an include guard. ... Or similar logic
> 
> Note that support for comments before the guard opens the door to support for 
> #pragma once. It is tempting to add that, but that is a slippery slope. I 
> would recommend leaving that as a defect that can be addressed later.
@djasper @klimek
Do you have any comments on this? I've begun to implement an enum/state machine 
but I'd like to know if you'd like me to implement a different way.


https://reviews.llvm.org/D35955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mark Zeren via Phabricator via cfe-commits]

mzeren-vmw added inline comments.



Comment at: unittests/Format/FormatTest.cpp:2281
 
 TEST_F(FormatTest, LayoutMacroDefinitionsStatementsSpanningBlocks) {
   verifyFormat("#define A \\\n"

mzeren-vmw wrote:
> Experimenting with the patch locally I found that comment indentation is 
> broken in some cases. Please add tests that cover comments. For example:
> 
> comment indented at code level as expected:
>   void f() {
>   #if 0
> // Comment
> code();
>   #  define FOO 0
>   #endif
>   }
> comment not indented at code level when there's a guard:
>   #ifndef _SOMEFILE_H
>   #define _SOMEFILE_H
>   void f() {
>   #if 0
>   // Comment
> code();
>   #  define FOO 0
>   #endif
>   }
>   #endif
> 
> The `#define FOO 0` is required to trigger this behavior.
Erik spent some time investigating issues with comment indentation. A couple of 
insights so far:

a) We need tests for wrapped macros (with trailing \ continuations). These need 
to include embedded comments.

b) It would be most natural to align comments with the following non-comment 
line. So a comment may be indented at "preprocessor level" or "code level". If 
indented at preprocessor level we have to handle the extra space introduced by 
the leading hash. This may require an extra bit per line to indicate whether 
the "aligned-to" line is preprocessor or code.


https://reviews.llvm.org/D35955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mark Zeren via Phabricator via cfe-commits]

mzeren-vmw added a comment.

Daniel's right. We need need substantially more tests!




Comment at: docs/ClangFormatStyleOptions.rst:1199
+  * ``PPDIS_AfterHash`` (in configuration: ``AfterHash``)
+Indents directives after the hash, counting the hash as a column.
+

delete outdated bit of description: ", counting the hash as a column"



Comment at: include/clang/Format/Format.h:1038
+PPDIS_None,
+/// Indents directives after the hash, counting the hash as a column.
+/// \code

ditto



Comment at: lib/Format/UnwrappedLineParser.cpp:707
 void UnwrappedLineParser::parsePPElse() {
+  // If a potential include guard has a #else, it's not an include guard.
+  if (FoundIncludeGuardStart && PPBranchLevel == 0)

... has an #else ...



Comment at: lib/Format/UnwrappedLineParser.cpp:728
+  if (FoundIncludeGuardStart && PPBranchLevel == -1 && PeekNext->is(tok::eof)) 
{
+for (auto it = Lines.begin(); it != Lines.end(); ++it) {
+  if (it->InPPDirective && it->Level > 0)

You can use a range based for loop here. Also locals begin with upper case:

   for (auto& Line : Lines)  {



Comment at: lib/Format/UnwrappedLineParser.cpp:742-744
+  if (PPMaybeIncludeGuard &&
+  PPMaybeIncludeGuard->TokenText == FormatTok->TokenText &&
+  Lines.size() == 1) {

Lines.size() == 1 is problematic. We must at least support comments before the 
include guard. I'll comment on the tests and point out a couple of important 
cases that currently fail.



Comment at: unittests/Format/FormatTest.cpp:2281
 
 TEST_F(FormatTest, LayoutMacroDefinitionsStatementsSpanningBlocks) {
   verifyFormat("#define A \\\n"

Experimenting with the patch locally I found that comment indentation is broken 
in some cases. Please add tests that cover comments. For example:

comment indented at code level as expected:
  void f() {
  #if 0
// Comment
code();
  #  define FOO 0
  #endif
  }
comment not indented at code level when there's a guard:
  #ifndef _SOMEFILE_H
  #define _SOMEFILE_H
  void f() {
  #if 0
  // Comment
code();
  #  define FOO 0
  #endif
  }
  #endif

The `#define FOO 0` is required to trigger this behavior.



Comment at: unittests/Format/FormatTest.cpp:2322-2331
+  // Test with include guards.
+  EXPECT_EQ("#ifndef _SOMEFILE_H\n"
+"#define _SOMEFILE_H\n"
+"code();\n"
+"#endif",
+format("#ifndef _SOMEFILE_H\n"
+   "#define _SOMEFILE_H\n"

A DRY-er way to say this is:
  auto WithGuard = "#ifndef _SOMEFILE_H\n"
   "#define _SOMEFILE_H\n"
   "code();\n"
   "#endif";
  ASSERT_EQ(WithGuards, format(WithGuards, Style));




Comment at: unittests/Format/FormatTest.cpp:2334
+  // after #ifndef.
+  EXPECT_EQ("#ifndef NOT_GUARD\n"
+"#  define FOO\n"


It would be nice to have a comment that explains why you cannot use 
verifyFormat.



Comment at: unittests/Format/FormatTest.cpp:2394
+  // previous code line yet, so we can't detect it.
+  EXPECT_NE("#ifndef NOT_GUARD\n"
+"code();\n"

The two "Defect:" cases would be more precise and more self-documenting if they 
used EXPECT_EQ where the "expected" text showed the incorrect results.



Comment at: unittests/Format/FormatTest.cpp:2405-2408
+  // Defect: We currently do not deal with cases where legitimate lines may be
+  // outside an include guard. Examples are #pragma once and
+  // #pragma GCC diagnostic, or anything else that does not change the meaning
+  // of the file if it's included multiple times.

We need to handle comments (like copyrights) before the include guard. There is 
an analogous problem with trailing blank lines, or trailing comments.

I think we need a small state machine: Init, Start, Open, Closed, NotGuard 
(terminal). `FoundIncludeGuardStart` should change from a bool to an enum to 
track this state. `PPMaybeIncludeGuard` can then drop it's "mabye". Whether or 
not it is null depends directly on the state. It does not determine state 
itself. While parsing, each line updates the state. If we get to `eof` in the 
Closed state then we have detected an include guard. ... Or similar logic

Note that support for comments before the guard opens the door to support for 
#pragma once. It is tempting to add that, but that is a slippery slope. I would 
recommend leaving that as a defect that can be addressed later.


https://reviews.llvm.org/D35955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

LG from my side.


https://reviews.llvm.org/D35955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erik Uhlmann via Phabricator via cfe-commits]

euhlmann updated this revision to Diff 110602.
euhlmann edited the summary of this revision.
euhlmann added a comment.

The patch now uses `PPBranchLevel` to track indent level. It allows 
`PPBranchLevel` to go back down to -1. The existing `PPBranchLevel >= 0` checks 
appear to prevent indexing into structures when `PPBranchLevel` is -1.


https://reviews.llvm.org/D35955

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  lib/Format/UnwrappedLineFormatter.cpp
  lib/Format/UnwrappedLineParser.cpp
  lib/Format/UnwrappedLineParser.h
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2286,8 +2286,158 @@
getLLVMStyleWithColumns(11));
 }
 
-TEST_F(FormatTest, IndentPreprocessorDirectivesAtZero) {
-  EXPECT_EQ("{\n  {\n#define A\n  }\n}", format("{{\n#define A\n}}"));
+TEST_F(FormatTest, IndentPreprocessorDirectives) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentPPDirectives = FormatStyle::PPDIS_None;
+  verifyFormat("#ifdef _WIN32\n"
+   "#define A 0\n"
+   "#ifdef VAR2\n"
+   "#define B 1\n"
+   "#include \n"
+   "#define MACRO  "
+   "\\\n"
+   "  some_very_long_func_a"
+   "a();\n"
+   "#endif\n"
+   "#else\n"
+   "#define A 1\n"
+   "#endif",
+   Style);
+
+  Style.IndentPPDirectives = FormatStyle::PPDIS_AfterHash;
+  verifyFormat("#ifdef _WIN32\n"
+   "#  define A 0\n"
+   "#  ifdef VAR2\n"
+   "#define B 1\n"
+   "#include \n"
+   "#define MACRO  "
+   "\\\n"
+   "  some_very_long_func_a"
+   "a();\n"
+   "#  endif\n"
+   "#else\n"
+   "#  define A 1\n"
+   "#endif",
+   Style);
+  // Test with include guards.
+  EXPECT_EQ("#ifndef _SOMEFILE_H\n"
+"#define _SOMEFILE_H\n"
+"code();\n"
+"#endif",
+format("#ifndef _SOMEFILE_H\n"
+   "#define _SOMEFILE_H\n"
+   "code();\n"
+   "#endif",
+   Style));
+  // Include guards must have a #define with the same variable immediately
+  // after #ifndef.
+  EXPECT_EQ("#ifndef NOT_GUARD\n"
+"#  define FOO\n"
+"code();\n"
+"#endif",
+format("#ifndef NOT_GUARD\n"
+   "#  define FOO\n"
+   "code();\n"
+   "#endif",
+   Style));
+
+  // Include guards must cover the entire file.
+  EXPECT_EQ("code();\n"
+"#ifndef NOT_GUARD\n"
+"#  define NOT_GUARD\n"
+"code();\n"
+"#endif",
+format("code();\n"
+   "#ifndef NOT_GUARD\n"
+   "#  define NOT_GUARD\n"
+   "code();\n"
+   "#endif",
+   Style));
+  EXPECT_EQ("#ifndef NOT_GUARD\n"
+"#  define NOT_GUARD\n"
+"code();\n"
+"#endif\n"
+"code();",
+format("#ifndef NOT_GUARD\n"
+   "#  define NOT_GUARD\n"
+   "code();\n"
+   "#endif\n"
+   "code();",
+   Style));
+  // Include guards don't have #else.
+  EXPECT_EQ("#ifndef NOT_GUARD\n"
+"#  define NOT_GUARD\n"
+"code();\n"
+"#else\n"
+"#endif",
+format("#ifndef NOT_GUARD\n"
+   "#  define NOT_GUARD\n"
+   "code();\n"
+   "#else\n"
+   "#endif",
+   Style));
+  EXPECT_EQ("#ifndef NOT_GUARD\n"
+"#  define NOT_GUARD\n"
+"code();\n"
+"#elif FOO\n"
+"#endif",
+format("#ifndef NOT_GUARD\n"
+   "#  define NOT_GUARD\n"
+   "code();\n"
+   "#elif FOO\n"
+   "#endif",
+   Style));
+  // Defect: We currently do not deal with the case where there's code between
+  // the #ifndef and #define but all other conditions hold. This is because when
+  // the #define line is parsed, UnwrappedLineParser::Lines doesn't hold the
+  // previous code line yet, so we can't detect it.
+  EXPECT_NE("#ifndef NOT_GUARD\n"
+"code();\n"
+"#  define NOT_GUARD\n"
+"code();\n"
+"#endif",
+  

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

[Erik Uhlmann via Phabricator via cfe-commits]

euhlmann updated this revision to Diff 110429.
euhlmann edited the summary of this revision.
euhlmann added a comment.

This addresses Daniel's previous concerns. The option is now an enum, the 
heuristic for detecting include guards is expanded and has corresponding unit 
tests, and style issues are resolved.
This does not yet use `PPBranchLevel` to track indent; I'm updating this review 
with my current work before addressing that point.


https://reviews.llvm.org/D35955

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  lib/Format/UnwrappedLineFormatter.cpp
  lib/Format/UnwrappedLineParser.cpp
  lib/Format/UnwrappedLineParser.h
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2286,8 +2286,158 @@
getLLVMStyleWithColumns(11));
 }
 
-TEST_F(FormatTest, IndentPreprocessorDirectivesAtZero) {
-  EXPECT_EQ("{\n  {\n#define A\n  }\n}", format("{{\n#define A\n}}"));
+TEST_F(FormatTest, IndentPreprocessorDirectives) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentPPDirectives = FormatStyle::PPDIS_None;
+  verifyFormat("#ifdef _WIN32\n"
+   "#define A 0\n"
+   "#ifdef VAR2\n"
+   "#define B 1\n"
+   "#include \n"
+   "#define MACRO  "
+   "\\\n"
+   "  some_very_long_func_a"
+   "a();\n"
+   "#endif\n"
+   "#else\n"
+   "#define A 1\n"
+   "#endif",
+   Style);
+
+  Style.IndentPPDirectives = FormatStyle::PPDIS_AfterHash;
+  verifyFormat("#ifdef _WIN32\n"
+   "#  define A 0\n"
+   "#  ifdef VAR2\n"
+   "#define B 1\n"
+   "#include \n"
+   "#define MACRO  "
+   "\\\n"
+   "  some_very_long_func_a"
+   "a();\n"
+   "#  endif\n"
+   "#else\n"
+   "#  define A 1\n"
+   "#endif",
+   Style);
+  // Test with include guards.
+  EXPECT_EQ("#ifndef _SOMEFILE_H\n"
+"#define _SOMEFILE_H\n"
+"code();\n"
+"#endif",
+format("#ifndef _SOMEFILE_H\n"
+   "#define _SOMEFILE_H\n"
+   "code();\n"
+   "#endif",
+   Style));
+  // Include guards must have a #define with the same variable immediately
+  // after #ifndef.
+  EXPECT_EQ("#ifndef NOT_GUARD\n"
+"#  define FOO\n"
+"code();\n"
+"#endif",
+format("#ifndef NOT_GUARD\n"
+   "#  define FOO\n"
+   "code();\n"
+   "#endif",
+   Style));
+
+  // Include guards must cover the entire file.
+  EXPECT_EQ("code();\n"
+"#ifndef NOT_GUARD\n"
+"#  define NOT_GUARD\n"
+"code();\n"
+"#endif",
+format("code();\n"
+   "#ifndef NOT_GUARD\n"
+   "#  define NOT_GUARD\n"
+   "code();\n"
+   "#endif",
+   Style));
+  EXPECT_EQ("#ifndef NOT_GUARD\n"
+"#  define NOT_GUARD\n"
+"code();\n"
+"#endif\n"
+"code();",
+format("#ifndef NOT_GUARD\n"
+   "#  define NOT_GUARD\n"
+   "code();\n"
+   "#endif\n"
+   "code();",
+   Style));
+  // Include guards don't have #else.
+  EXPECT_EQ("#ifndef NOT_GUARD\n"
+"#  define NOT_GUARD\n"
+"code();\n"
+"#else\n"
+"#endif",
+format("#ifndef NOT_GUARD\n"
+   "#  define NOT_GUARD\n"
+   "code();\n"
+   "#else\n"
+   "#endif",
+   Style));
+  EXPECT_EQ("#ifndef NOT_GUARD\n"
+"#  define NOT_GUARD\n"
+"code();\n"
+"#elif FOO\n"
+"#endif",
+format("#ifndef NOT_GUARD\n"
+   "#  define NOT_GUARD\n"
+   "code();\n"
+   "#elif FOO\n"
+   "#endif",
+   Style));
+  // Defect: We currently do not deal with the case where there's code between
+  // the #ifndef and #define but all other conditions hold. This is because when
+  // the #define line is parsed, UnwrappedLineParser::Lines doesn't hold the
+  // previous code line yet, so we can't detect it.
+  EXPECT_NE("#ifndef NOT_GUARD\n"
+ 

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

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In https://reviews.llvm.org/D35955#835994, @euhlmann wrote:

> In https://reviews.llvm.org/D35955#835439, @klimek wrote:
>
> > I think if we need this info, we can just make it count down to -1 again 
> > (or, but that's isomorphic, let it run from 0 and make sure we never index 
> > into the data structures at 0 :)
>
>
> Should I do one of these things? Let me know how you'd like me to implement 
> this.


So my suggestion would be to let it count down to -1 again and put a check 
around indexing into the data structures.


https://reviews.llvm.org/D35955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erik Uhlmann via Phabricator via cfe-commits]

euhlmann added a comment.

In https://reviews.llvm.org/D35955#835439, @klimek wrote:

> I think if we need this info, we can just make it count down to -1 again (or, 
> but that's isomorphic, let it run from 0 and make sure we never index into 
> the data structures at 0 :)


Should I do one of these things? Let me know how you'd like me to implement 
this.


https://reviews.llvm.org/D35955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In https://reviews.llvm.org/D35955#834914, @djasper wrote:

> Manuel: Can you take a look at the last comment here? Why does PPBranchLevel 
> start at -1?


It's a perhaps too-clever implementation to make sure that we can have a 
per-branch data structure (indexed from 0) , and then making sure that we never 
index out of bounds by never going down to -1 again.
I think if we need this info, we can just make it count down to -1 again (or, 
but that's isomorphic, let it run from 0 and make sure we never index into the 
data structures at 0 :)


https://reviews.llvm.org/D35955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

Manuel: Can you take a look at the last comment here? Why does PPBranchLevel 
start at -1?


https://reviews.llvm.org/D35955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erik Uhlmann via Phabricator via cfe-commits]

euhlmann added inline comments.



Comment at: lib/Format/UnwrappedLineParser.h:238
 
+  unsigned PPIndentLevel;
+  FormatToken *PPMaybeIncludeGuard;

djasper wrote:
> I think this should almost always be PPBranchLevel. Probably the different 
> case is the #else itself, but I think we can handle that easily.
It doesn't look like PPBranchLevel can be used to differentiate between no 
indent and one level of indent, since it starts at -1 and then stays >=0. For 
example:
```
#define A // -1
#if B // -1
#  define C   // 0
#  if D   // 0
#define E // 1
#  endif  // 0
#endif// 0
#define F // 0
```



https://reviews.llvm.org/D35955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erik Uhlmann via Phabricator via cfe-commits]

euhlmann added inline comments.



Comment at: lib/Format/UnwrappedLineParser.cpp:701
+  PPMaybeIncludeGuard->TokenText == FormatTok->TokenText &&
+  PPIndentLevel > 0) {
+--PPIndentLevel;

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?


https://reviews.llvm.org/D35955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erik Uhlmann via Phabricator via cfe-commits]

euhlmann 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;

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.


https://reviews.llvm.org/D35955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erik Uhlmann via Phabricator via cfe-commits]

euhlmann created this revision.
euhlmann added a project: clang.

This is an implementation for bug 17362 
 which adds support for 
indenting preprocessor statements inside if/ifdef/endif. This takes previous 
work from fmauch and makes it into a full feature.
The context of this patch is that I'm a VMware intern, and I implemented this 
because VMware needs the feature. As such, some decisions were made based on 
what VMware wants, and I would appreciate suggestions on expanding this if 
necessary to use-cases other people may want.

Example:

  #if FOO #if FOO
  #define BAR   -># define BAR
  #endif  #endif



- It's controlled by a new bool config option `IndentPPDirectives`
- Indenting happens after the hash but the hash counts as a column. In other 
words, one space is inserted for the first indent level and two spaces for 
every other level.

  #if FOO
  # if BAR
  #   include 
  # endif
  #endif

This suited VMware's needs, but if not counting the hash as a column is 
significant, this could be configurable instead.

- Tabs are supported if enabled with `UseTab`
- There's a heuristic for detecting include guards and not indenting them. It 
looks for the pattern

  #ifndef 

immediately followed by

  #define 

This was chosen because it's simple, but it will cause problems if that pattern 
appears when it's not meant to be an include guard.


https://reviews.llvm.org/D35955

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  lib/Format/UnwrappedLineFormatter.cpp
  lib/Format/UnwrappedLineParser.cpp
  lib/Format/UnwrappedLineParser.h
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2215,8 +2215,55 @@
getLLVMStyleWithColumns(11));
 }
 
-TEST_F(FormatTest, IndentPreprocessorDirectivesAtZero) {
-  EXPECT_EQ("{\n  {\n#define A\n  }\n}", format("{{\n#define A\n}}"));
+TEST_F(FormatTest, IndentPreprocessorDirectives) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentPPDirectives = false;
+  verifyFormat("#ifdef _WIN32\n"
+   "#define A 0\n"
+   "#ifdef VAR2\n"
+   "#define B 1\n"
+   "#include \n"
+   "#define MACRO  "
+   "\\\n"
+   "  some_very_long_func_a"
+   "a();\n"
+   "#endif\n"
+   "#else\n"
+   "#define A 1\n"
+   "#endif", Style);
+
+  Style.IndentPPDirectives = true;
+  verifyFormat("#ifdef _WIN32\n"
+   "# define A 0\n"
+   "# ifdef VAR2\n"
+   "#   define B 1\n"
+   "#   include \n"
+   "#   define MACRO   "
+   "\\\n"
+   "  some_very_long_func_a"
+   "a();\n"
+   "# endif\n"
+   "#else\n"
+   "# define A 1\n"
+   "#endif", Style);
+  // don't indent include guards
+  // verifyFormat() currently calls test::messUp() which incorrectly merges L2
+  // and L3, so call EXPECT_EQ manually
+  EXPECT_EQ("#ifndef _SOMEFILE_H\n"
+"#define _SOMEFILE_H\n"
+"code();\n"
+"#endif", format("#ifndef _SOMEFILE_H\n"
+ "#define _SOMEFILE_H\n"
+ "code();\n"
+ "#endif", Style));
+
+  // make sure it works even with tabs
+  Style.UseTab = FormatStyle::UT_Always;
+  Style.IndentWidth = 8;
+  Style.TabWidth = 8;
+  verifyFormat("#ifdef _WIN32\n"
+   "#\tdefine C 1\n"
+   "#endif", Style);
 }
 
 TEST_F(FormatTest, FormatHashIfNotAtStartOfLine) {
Index: lib/Format/UnwrappedLineParser.h
===
--- lib/Format/UnwrappedLineParser.h
+++ lib/Format/UnwrappedLineParser.h
@@ -235,6 +235,9 @@
   // sequence.
   std::stack PPChainBranchIndex;
 
+  unsigned PPIndentLevel;
+  FormatToken *PPMaybeIncludeGuard;
+
   friend class ScopedLineState;
   friend class CompoundStatementIndenter;
 };
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -231,7 +231,8 @@
 : Line(new UnwrappedLine), MustBreakBeforeNextToken(false),
   CurrentLines(), Style(Style), Keywords(Keywords),
   CommentPragmasRegex(Style.CommentPragmas), Tokens(nullptr),
-  Callback(Callback), AllTokens(Tokens), PPBranchLevel(-1) {}
+  Callback(Callback),