[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

[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

[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

[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

[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

[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

[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

[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] D35847: clang-format: Fix left pointer alignment after delctype/typeof

[Erik Uhlmann via Phabricator via cfe-commits]

euhlmann marked 2 inline comments as done.
euhlmann added a comment.

I resolved the formatting issues. I apologize for not paying closer attention 
to formatting earlier.

I don't have commit access, so if this change looks good now, could someone 
with access please commit?


https://reviews.llvm.org/D35847



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

[PATCH] D35847: clang-format: Fix left pointer alignment after delctype/typeof

[Erik Uhlmann via Phabricator via cfe-commits]

euhlmann updated this revision to Diff 110032.
euhlmann added a comment.

Ran clang-format over changes and corrected formatting


https://reviews.llvm.org/D35847

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -5422,6 +5422,10 @@
   verifyFormat("for (;; *a = b) {\n}", Left);
   verifyFormat("return *this += 1;", Left);
   verifyFormat("throw *x;", Left);
+  verifyFormat("delete *x;", Left);
+  verifyFormat("typedef typeof(int(int, int))* MyFuncPtr;", Left);
+  verifyFormat("[](const decltype(*a)* ptr) {}", Left);
+  verifyFormat("typedef typeof /*comment*/ (int(int, int))* MyFuncPtr;", Left);
 
   verifyIndependentOfContext("a = *(x + y);");
   verifyIndependentOfContext("a = &(x + y);");
@@ -5468,9 +5472,6 @@
   verifyGoogleFormat("T** t = new T*;");
   verifyGoogleFormat("T** t = new T*();");
 
-  FormatStyle PointerLeft = getLLVMStyle();
-  PointerLeft.PointerAlignment = FormatStyle::PAS_Left;
-  verifyFormat("delete *x;", PointerLeft);
   verifyFormat("STATIC_ASSERT((a & b) == 0);");
   verifyFormat("STATIC_ASSERT(0 == (a & b));");
   verifyFormat("template  "
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -1397,11 +1397,13 @@
 if (NextToken->isOneOf(tok::comma, tok::semi))
   return TT_PointerOrReference;
 
-if (PrevToken->is(tok::r_paren) && PrevToken->MatchingParen &&
-PrevToken->MatchingParen->Previous &&
-PrevToken->MatchingParen->Previous->isOneOf(tok::kw_typeof,
-tok::kw_decltype))
-  return TT_PointerOrReference;
+if (PrevToken->is(tok::r_paren) && PrevToken->MatchingParen) {
+  FormatToken *TokenBeforeMatchingParen =
+  PrevToken->MatchingParen->getPreviousNonComment();
+  if (TokenBeforeMatchingParen &&
+  TokenBeforeMatchingParen->isOneOf(tok::kw_typeof, tok::kw_decltype))
+return TT_PointerOrReference;
+}
 
 if (PrevToken->Tok.isLiteral() ||
 PrevToken->isOneOf(tok::r_paren, tok::r_square, tok::kw_true,
@@ -2205,15 +2207,25 @@
 Left.Previous->is(tok::kw_case));
   if (Left.is(tok::l_square) && Right.is(tok::amp))
 return false;
-  if (Right.is(TT_PointerOrReference))
-return (Left.is(tok::r_paren) && Line.MightBeFunctionDecl) ||
-   (Left.Tok.isLiteral() || (Left.is(tok::kw_const) && Left.Previous &&
- Left.Previous->is(tok::r_paren)) ||
+  if (Right.is(TT_PointerOrReference)) {
+if (Left.is(tok::r_paren) && Line.MightBeFunctionDecl) {
+  if (!Left.MatchingParen)
+return true;
+  FormatToken *TokenBeforeMatchingParen =
+  Left.MatchingParen->getPreviousNonComment();
+  if (!TokenBeforeMatchingParen ||
+  !TokenBeforeMatchingParen->isOneOf(tok::kw_typeof, tok::kw_decltype))
+return true;
+}
+return (Left.Tok.isLiteral() ||
+(Left.is(tok::kw_const) && Left.Previous &&
+ Left.Previous->is(tok::r_paren)) ||
 (!Left.isOneOf(TT_PointerOrReference, tok::l_paren) &&
  (Style.PointerAlignment != FormatStyle::PAS_Left ||
   (Line.IsMultiVariableDeclStmt &&
(Left.NestingLevel == 0 ||
 (Left.NestingLevel == 1 && Line.First->is(tok::kw_for)));
+  }
   if (Right.is(TT_FunctionTypeLParen) && Left.isNot(tok::l_paren) &&
   (!Left.is(TT_PointerOrReference) ||
(Style.PointerAlignment != FormatStyle::PAS_Right &&


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -5422,6 +5422,10 @@
   verifyFormat("for (;; *a = b) {\n}", Left);
   verifyFormat("return *this += 1;", Left);
   verifyFormat("throw *x;", Left);
+  verifyFormat("delete *x;", Left);
+  verifyFormat("typedef typeof(int(int, int))* MyFuncPtr;", Left);
+  verifyFormat("[](const decltype(*a)* ptr) {}", Left);
+  verifyFormat("typedef typeof /*comment*/ (int(int, int))* MyFuncPtr;", Left);
 
   verifyIndependentOfContext("a = *(x + y);");
   verifyIndependentOfContext("a = &(x + y);");
@@ -5468,9 +5472,6 @@
   verifyGoogleFormat("T** t = new T*;");
   verifyGoogleFormat("T** t = new T*();");
 
-  FormatStyle PointerLeft = getLLVMStyle();
-  PointerLeft.PointerAlignment = FormatStyle::PAS_Left;
-  verifyFormat("delete *x;", PointerLeft);
   verifyFormat("STATIC_ASSERT((a & b) == 0);");
   verifyFormat("STATIC_ASSERT(0 == (a & b));");
   verifyFormat("template  "
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ 

[PATCH] D35847: clang-format: Fix left pointer alignment after delctype/typeof

[Erik Uhlmann via Phabricator via cfe-commits]

euhlmann updated this revision to Diff 109805.
euhlmann added a comment.

Fix bad formatting


https://reviews.llvm.org/D35847

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -5422,6 +5422,10 @@
   verifyFormat("for (;; *a = b) {\n}", Left);
   verifyFormat("return *this += 1;", Left);
   verifyFormat("throw *x;", Left);
+  verifyFormat("delete *x;", Left);
+  verifyFormat("typedef typeof(int(int, int))* MyFuncPtr;", Left);
+  verifyFormat("[](const decltype(*a)* ptr) {}", Left);
+  verifyFormat("typedef typeof /*comment*/ (int(int, int))* MyFuncPtr;", Left);
 
   verifyIndependentOfContext("a = *(x + y);");
   verifyIndependentOfContext("a = &(x + y);");
@@ -5468,9 +5472,6 @@
   verifyGoogleFormat("T** t = new T*;");
   verifyGoogleFormat("T** t = new T*();");
 
-  FormatStyle PointerLeft = getLLVMStyle();
-  PointerLeft.PointerAlignment = FormatStyle::PAS_Left;
-  verifyFormat("delete *x;", PointerLeft);
   verifyFormat("STATIC_ASSERT((a & b) == 0);");
   verifyFormat("STATIC_ASSERT(0 == (a & b));");
   verifyFormat("template  "
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -1397,11 +1397,13 @@
 if (NextToken->isOneOf(tok::comma, tok::semi))
   return TT_PointerOrReference;
 
-if (PrevToken->is(tok::r_paren) && PrevToken->MatchingParen &&
-PrevToken->MatchingParen->Previous &&
-PrevToken->MatchingParen->Previous->isOneOf(tok::kw_typeof,
-tok::kw_decltype))
-  return TT_PointerOrReference;
+if (PrevToken->is(tok::r_paren) && PrevToken->MatchingParen) {
+  FormatToken *TokenBeforeMatchingParen =
+PrevToken->MatchingParen->getPreviousNonComment();
+  if (TokenBeforeMatchingParen &&
+  TokenBeforeMatchingParen->isOneOf(tok::kw_typeof, tok::kw_decltype))
+return TT_PointerOrReference;
+}
 
 if (PrevToken->Tok.isLiteral() ||
 PrevToken->isOneOf(tok::r_paren, tok::r_square, tok::kw_true,
@@ -2205,15 +2207,24 @@
 Left.Previous->is(tok::kw_case));
   if (Left.is(tok::l_square) && Right.is(tok::amp))
 return false;
-  if (Right.is(TT_PointerOrReference))
-return (Left.is(tok::r_paren) && Line.MightBeFunctionDecl) ||
-   (Left.Tok.isLiteral() || (Left.is(tok::kw_const) && Left.Previous &&
+  if (Right.is(TT_PointerOrReference)) {
+if (Left.is(tok::r_paren) && Line.MightBeFunctionDecl) {
+  if (!Left.MatchingParen)
+return true;
+  FormatToken *TokenBeforeMatchingParen =
+Left.MatchingParen->getPreviousNonComment();
+  if (!TokenBeforeMatchingParen ||
+  !TokenBeforeMatchingParen->isOneOf(tok::kw_typeof, tok::kw_decltype))
+return true;
+}
+return (Left.Tok.isLiteral() || (Left.is(tok::kw_const) && Left.Previous &&
  Left.Previous->is(tok::r_paren)) ||
 (!Left.isOneOf(TT_PointerOrReference, tok::l_paren) &&
  (Style.PointerAlignment != FormatStyle::PAS_Left ||
   (Line.IsMultiVariableDeclStmt &&
(Left.NestingLevel == 0 ||
 (Left.NestingLevel == 1 && Line.First->is(tok::kw_for)));
+  }
   if (Right.is(TT_FunctionTypeLParen) && Left.isNot(tok::l_paren) &&
   (!Left.is(TT_PointerOrReference) ||
(Style.PointerAlignment != FormatStyle::PAS_Right &&


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -5422,6 +5422,10 @@
   verifyFormat("for (;; *a = b) {\n}", Left);
   verifyFormat("return *this += 1;", Left);
   verifyFormat("throw *x;", Left);
+  verifyFormat("delete *x;", Left);
+  verifyFormat("typedef typeof(int(int, int))* MyFuncPtr;", Left);
+  verifyFormat("[](const decltype(*a)* ptr) {}", Left);
+  verifyFormat("typedef typeof /*comment*/ (int(int, int))* MyFuncPtr;", Left);
 
   verifyIndependentOfContext("a = *(x + y);");
   verifyIndependentOfContext("a = &(x + y);");
@@ -5468,9 +5472,6 @@
   verifyGoogleFormat("T** t = new T*;");
   verifyGoogleFormat("T** t = new T*();");
 
-  FormatStyle PointerLeft = getLLVMStyle();
-  PointerLeft.PointerAlignment = FormatStyle::PAS_Left;
-  verifyFormat("delete *x;", PointerLeft);
   verifyFormat("STATIC_ASSERT((a & b) == 0);");
   verifyFormat("STATIC_ASSERT(0 == (a & b));");
   verifyFormat("template  "
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -1397,11 +1397,13 @@
 if (NextToken->isOneOf(tok::comma, tok::semi))
  

[PATCH] D35847: clang-format: Fix left pointer alignment after delctype/typeof

[Erik Uhlmann via Phabricator via cfe-commits]

euhlmann updated this revision to Diff 109785.
euhlmann added a comment.

Pulled out `getPreviousNonComment()` into local variable to avoid calling twice.


https://reviews.llvm.org/D35847

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -5422,6 +5422,10 @@
   verifyFormat("for (;; *a = b) {\n}", Left);
   verifyFormat("return *this += 1;", Left);
   verifyFormat("throw *x;", Left);
+  verifyFormat("delete *x;", Left);
+  verifyFormat("typedef typeof(int(int, int))* MyFuncPtr;", Left);
+  verifyFormat("[](const decltype(*a)* ptr) {}", Left);
+  verifyFormat("typedef typeof /*comment*/ (int(int, int))* MyFuncPtr;", Left);
 
   verifyIndependentOfContext("a = *(x + y);");
   verifyIndependentOfContext("a = &(x + y);");
@@ -5468,9 +5472,6 @@
   verifyGoogleFormat("T** t = new T*;");
   verifyGoogleFormat("T** t = new T*();");
 
-  FormatStyle PointerLeft = getLLVMStyle();
-  PointerLeft.PointerAlignment = FormatStyle::PAS_Left;
-  verifyFormat("delete *x;", PointerLeft);
   verifyFormat("STATIC_ASSERT((a & b) == 0);");
   verifyFormat("STATIC_ASSERT(0 == (a & b));");
   verifyFormat("template  "
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -1397,11 +1397,15 @@
 if (NextToken->isOneOf(tok::comma, tok::semi))
   return TT_PointerOrReference;
 
-if (PrevToken->is(tok::r_paren) && PrevToken->MatchingParen &&
-PrevToken->MatchingParen->Previous &&
-PrevToken->MatchingParen->Previous->isOneOf(tok::kw_typeof,
-tok::kw_decltype))
-  return TT_PointerOrReference;
+if (PrevToken->is(tok::r_paren) && PrevToken->MatchingParen) {
+  FormatToken *TokenBeforeMatchingParen =
+PrevToken->MatchingParen->getPreviousNonComment();
+  if (TokenBeforeMatchingParen &&
+  TokenBeforeMatchingParen->isOneOf(
+tok::kw_typeof,
+tok::kw_decltype))
+return TT_PointerOrReference;
+}
 
 if (PrevToken->Tok.isLiteral() ||
 PrevToken->isOneOf(tok::r_paren, tok::r_square, tok::kw_true,
@@ -2205,15 +2209,24 @@
 Left.Previous->is(tok::kw_case));
   if (Left.is(tok::l_square) && Right.is(tok::amp))
 return false;
-  if (Right.is(TT_PointerOrReference))
-return (Left.is(tok::r_paren) && Line.MightBeFunctionDecl) ||
-   (Left.Tok.isLiteral() || (Left.is(tok::kw_const) && Left.Previous &&
+  if (Right.is(TT_PointerOrReference)) {
+if (Left.is(tok::r_paren) && Line.MightBeFunctionDecl) {
+  if (!Left.MatchingParen)
+return true;
+  FormatToken *TokenBeforeMatchingParen =
+Left.MatchingParen->getPreviousNonComment();
+  if (!TokenBeforeMatchingParen ||
+  !TokenBeforeMatchingParen->isOneOf(tok::kw_typeof, tok::kw_decltype))
+return true;
+}
+return (Left.Tok.isLiteral() || (Left.is(tok::kw_const) && Left.Previous &&
  Left.Previous->is(tok::r_paren)) ||
 (!Left.isOneOf(TT_PointerOrReference, tok::l_paren) &&
  (Style.PointerAlignment != FormatStyle::PAS_Left ||
   (Line.IsMultiVariableDeclStmt &&
(Left.NestingLevel == 0 ||
 (Left.NestingLevel == 1 && Line.First->is(tok::kw_for)));
+  }
   if (Right.is(TT_FunctionTypeLParen) && Left.isNot(tok::l_paren) &&
   (!Left.is(TT_PointerOrReference) ||
(Style.PointerAlignment != FormatStyle::PAS_Right &&


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -5422,6 +5422,10 @@
   verifyFormat("for (;; *a = b) {\n}", Left);
   verifyFormat("return *this += 1;", Left);
   verifyFormat("throw *x;", Left);
+  verifyFormat("delete *x;", Left);
+  verifyFormat("typedef typeof(int(int, int))* MyFuncPtr;", Left);
+  verifyFormat("[](const decltype(*a)* ptr) {}", Left);
+  verifyFormat("typedef typeof /*comment*/ (int(int, int))* MyFuncPtr;", Left);
 
   verifyIndependentOfContext("a = *(x + y);");
   verifyIndependentOfContext("a = &(x + y);");
@@ -5468,9 +5472,6 @@
   verifyGoogleFormat("T** t = new T*;");
   verifyGoogleFormat("T** t = new T*();");
 
-  FormatStyle PointerLeft = getLLVMStyle();
-  PointerLeft.PointerAlignment = FormatStyle::PAS_Left;
-  verifyFormat("delete *x;", PointerLeft);
   verifyFormat("STATIC_ASSERT((a & b) == 0);");
   verifyFormat("STATIC_ASSERT(0 == (a & b));");
   verifyFormat("template  "
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ 

[PATCH] D35847: clang-format: Fix left pointer alignment after delctype/typeof

[Erik Uhlmann via Phabricator via cfe-commits]

euhlmann updated this revision to Diff 109613.
euhlmann added a comment.

This uses `FormatToken::getPreviousNonComment` and adds a test. This also fixes 
a bug in token annotation that was breaking the test (by also using 
`getPreviousNonComment` instead of `Previous`)


https://reviews.llvm.org/D35847

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -5422,6 +5422,10 @@
   verifyFormat("for (;; *a = b) {\n}", Left);
   verifyFormat("return *this += 1;", Left);
   verifyFormat("throw *x;", Left);
+  verifyFormat("delete *x;", Left);
+  verifyFormat("typedef typeof(int(int, int))* MyFuncPtr;", Left);
+  verifyFormat("[](const decltype(*a)* ptr) {}", Left);
+  verifyFormat("typedef typeof /*comment*/ (int(int, int))* MyFuncPtr;", Left);
 
   verifyIndependentOfContext("a = *(x + y);");
   verifyIndependentOfContext("a = &(x + y);");
@@ -5468,9 +5472,6 @@
   verifyGoogleFormat("T** t = new T*;");
   verifyGoogleFormat("T** t = new T*();");
 
-  FormatStyle PointerLeft = getLLVMStyle();
-  PointerLeft.PointerAlignment = FormatStyle::PAS_Left;
-  verifyFormat("delete *x;", PointerLeft);
   verifyFormat("STATIC_ASSERT((a & b) == 0);");
   verifyFormat("STATIC_ASSERT(0 == (a & b));");
   verifyFormat("template  "
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -1398,9 +1398,10 @@
   return TT_PointerOrReference;
 
 if (PrevToken->is(tok::r_paren) && PrevToken->MatchingParen &&
-PrevToken->MatchingParen->Previous &&
-PrevToken->MatchingParen->Previous->isOneOf(tok::kw_typeof,
-tok::kw_decltype))
+PrevToken->MatchingParen->getPreviousNonComment() &&
+PrevToken->MatchingParen->getPreviousNonComment()->isOneOf(
+  tok::kw_typeof,
+  tok::kw_decltype))
   return TT_PointerOrReference;
 
 if (PrevToken->Tok.isLiteral() ||
@@ -2206,7 +2207,12 @@
   if (Left.is(tok::l_square) && Right.is(tok::amp))
 return false;
   if (Right.is(TT_PointerOrReference))
-return (Left.is(tok::r_paren) && Line.MightBeFunctionDecl) ||
+return (Left.is(tok::r_paren) && Line.MightBeFunctionDecl &&
+!(Left.MatchingParen &&
+  Left.MatchingParen->getPreviousNonComment() &&
+  Left.MatchingParen->getPreviousNonComment()->isOneOf(
+tok::kw_typeof,
+tok::kw_decltype))) ||
(Left.Tok.isLiteral() || (Left.is(tok::kw_const) && Left.Previous &&
  Left.Previous->is(tok::r_paren)) ||
 (!Left.isOneOf(TT_PointerOrReference, tok::l_paren) &&


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -5422,6 +5422,10 @@
   verifyFormat("for (;; *a = b) {\n}", Left);
   verifyFormat("return *this += 1;", Left);
   verifyFormat("throw *x;", Left);
+  verifyFormat("delete *x;", Left);
+  verifyFormat("typedef typeof(int(int, int))* MyFuncPtr;", Left);
+  verifyFormat("[](const decltype(*a)* ptr) {}", Left);
+  verifyFormat("typedef typeof /*comment*/ (int(int, int))* MyFuncPtr;", Left);
 
   verifyIndependentOfContext("a = *(x + y);");
   verifyIndependentOfContext("a = &(x + y);");
@@ -5468,9 +5472,6 @@
   verifyGoogleFormat("T** t = new T*;");
   verifyGoogleFormat("T** t = new T*();");
 
-  FormatStyle PointerLeft = getLLVMStyle();
-  PointerLeft.PointerAlignment = FormatStyle::PAS_Left;
-  verifyFormat("delete *x;", PointerLeft);
   verifyFormat("STATIC_ASSERT((a & b) == 0);");
   verifyFormat("STATIC_ASSERT(0 == (a & b));");
   verifyFormat("template  "
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -1398,9 +1398,10 @@
   return TT_PointerOrReference;
 
 if (PrevToken->is(tok::r_paren) && PrevToken->MatchingParen &&
-PrevToken->MatchingParen->Previous &&
-PrevToken->MatchingParen->Previous->isOneOf(tok::kw_typeof,
-tok::kw_decltype))
+PrevToken->MatchingParen->getPreviousNonComment() &&
+PrevToken->MatchingParen->getPreviousNonComment()->isOneOf(
+  tok::kw_typeof,
+  tok::kw_decltype))
   return TT_PointerOrReference;
 
 if (PrevToken->Tok.isLiteral() ||
@@ -2206,7 +2207,12 @@
   if (Left.is(tok::l_square) && Right.is(tok::amp))
 return false;
   if (Right.is(TT_PointerOrReference))
-return (Left.is(tok::r_paren) && Line.MightBeFunctionDecl) ||
+return (Left.is(tok::r_paren) && 

[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

[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), 

[PATCH] D35847: clang-format: Fix left pointer alignment after delctype/typeof

[Erik Uhlmann via Phabricator via cfe-commits]

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

Change 272124* introduced a regression in spaceRequiredBetween for left aligned 
pointers to decltype and typeof expressions. This fix adds logic to fix this. 
The test added is based on a related test in determineStarAmpUsage. Also add 
test cases for the regression.

- http://llvm.org/viewvc/llvm-project?view=revision=272124

LLVM bug tracker: https://bugs.llvm.org/show_bug.cgi?id=30407


https://reviews.llvm.org/D35847

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -5351,6 +5351,9 @@
   verifyFormat("for (;; *a = b) {\n}", Left);
   verifyFormat("return *this += 1;", Left);
   verifyFormat("throw *x;", Left);
+  verifyFormat("delete *x;", Left);
+  verifyFormat("typedef typeof(int(int, int))* MyFuncPtr;", Left);
+  verifyFormat("[](const decltype(*a)* ptr) {}", Left);
 
   verifyIndependentOfContext("a = *(x + y);");
   verifyIndependentOfContext("a = &(x + y);");
@@ -5397,9 +5400,6 @@
   verifyGoogleFormat("T** t = new T*;");
   verifyGoogleFormat("T** t = new T*();");
 
-  FormatStyle PointerLeft = getLLVMStyle();
-  PointerLeft.PointerAlignment = FormatStyle::PAS_Left;
-  verifyFormat("delete *x;", PointerLeft);
   verifyFormat("STATIC_ASSERT((a & b) == 0);");
   verifyFormat("STATIC_ASSERT(0 == (a & b));");
   verifyFormat("template  "
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2202,7 +2202,10 @@
   if (Left.is(tok::l_square) && Right.is(tok::amp))
 return false;
   if (Right.is(TT_PointerOrReference))
-return (Left.is(tok::r_paren) && Line.MightBeFunctionDecl) ||
+return (Left.is(tok::r_paren) && Line.MightBeFunctionDecl &&
+!(Left.MatchingParen && Left.MatchingParen->Previous &&
+  Left.MatchingParen->Previous->isOneOf(tok::kw_typeof,
+tok::kw_decltype))) ||
(Left.Tok.isLiteral() || (Left.is(tok::kw_const) && Left.Previous &&
  Left.Previous->is(tok::r_paren)) ||
 (!Left.isOneOf(TT_PointerOrReference, tok::l_paren) &&


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -5351,6 +5351,9 @@
   verifyFormat("for (;; *a = b) {\n}", Left);
   verifyFormat("return *this += 1;", Left);
   verifyFormat("throw *x;", Left);
+  verifyFormat("delete *x;", Left);
+  verifyFormat("typedef typeof(int(int, int))* MyFuncPtr;", Left);
+  verifyFormat("[](const decltype(*a)* ptr) {}", Left);
 
   verifyIndependentOfContext("a = *(x + y);");
   verifyIndependentOfContext("a = &(x + y);");
@@ -5397,9 +5400,6 @@
   verifyGoogleFormat("T** t = new T*;");
   verifyGoogleFormat("T** t = new T*();");
 
-  FormatStyle PointerLeft = getLLVMStyle();
-  PointerLeft.PointerAlignment = FormatStyle::PAS_Left;
-  verifyFormat("delete *x;", PointerLeft);
   verifyFormat("STATIC_ASSERT((a & b) == 0);");
   verifyFormat("STATIC_ASSERT(0 == (a & b));");
   verifyFormat("template  "
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2202,7 +2202,10 @@
   if (Left.is(tok::l_square) && Right.is(tok::amp))
 return false;
   if (Right.is(TT_PointerOrReference))
-return (Left.is(tok::r_paren) && Line.MightBeFunctionDecl) ||
+return (Left.is(tok::r_paren) && Line.MightBeFunctionDecl &&
+!(Left.MatchingParen && Left.MatchingParen->Previous &&
+  Left.MatchingParen->Previous->isOneOf(tok::kw_typeof,
+tok::kw_decltype))) ||
(Left.Tok.isLiteral() || (Left.is(tok::kw_const) && Left.Previous &&
  Left.Previous->is(tok::r_paren)) ||
 (!Left.isOneOf(TT_PointerOrReference, tok::l_paren) &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits