[PATCH] D88227: [clang-format] Add a SpaceBeforePointerQualifiers style option

[Mark Zeren via Phabricator via cfe-commits]

mzeren-vmw added a comment.

Perhaps PointerAlignment should be an enum of Left, Middle, Right?
We maintain an internal patch that adds "ReferenceAlignment" as separate from 
PointerAlignment. So one could have:

  PointerAlignment: Middle
  ReferenceAlignment: Left


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88227/new/

https://reviews.llvm.org/D88227

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

[PATCH] D42034: [clang-format] In tests, expected code should be format-stable

[Mark Zeren via Phabricator via cfe-commits]

mzeren-vmw added inline comments.



Comment at: cfe/trunk/unittests/Format/FormatTest.cpp:78
 EXPECT_EQ(Expected.str(), format(Code, Style));
 if (Style.Language == FormatStyle::LK_Cpp) {
   // Objective-C++ is a superset of C++, so everything checked for C++

MyDeveloperDay wrote:
> @ mzeren-vmw just a minor comment related to a change you made last year.
> 
> When FormatTest  fails, all we are told is that it fails on line 75,or 77 
> (currently in trunk its 48,49) for every failure, with so many test cases 
> written as "foo()" or "aa()" it can often be hard to pinpoint the exact 
> test failure.
> 
> If verifyFormat was given an additional default argument of `int line`
> 
> Then verifyFormat could be used via a macro
> 
> ```
> #define VERIFYFORMAT(expected,code,style)
>  verifyFormat(expected,code,style,__LINE__);
> ```
> 
> The line numbers could then be passed as an additional failure message to 
> gtest by passing the __LINE__ of the test location down.
> 
> ```
> void verifyFormat(llvm::StringRef Expected, llvm::StringRef Code,
> const FormatStyle  = getLLVMStyle(),int 
> line=__LINE__) {
> EXPECT_EQ(Expected.str(), format(Expected, Style))
> << "Expected code is not stable at " << line;
> EXPECT_EQ(Expected.str(), format(Code, Style) << " at " << line;
> }
> ```
> 
> When the test fails we'd know the exact line of the test case that failed.
> 
> Also because of this, we get 2 failures for every failure, the second will 
> almost always fail as well if the first case does (from what I can tell), is 
> there anyway we can query the first failed result and not bother doing the 
> second if the first one failed?
All good points. I don't think I'll be able to get to this in the near future.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D42034/new/

https://reviews.llvm.org/D42034



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Mark Zeren via Phabricator via cfe-commits]

mzeren-vmw added a comment.

In D55170#1436087 , @klimek wrote:

> Oh wow, wasn't aware how popular this is. Sigh. I'd like us to push them to 
> change their style guide, but I guess that's not going to fly.


IDK. If the criteria is "public coding standard for widely used project", 
supporting all such things may be unsustainable. Some projects may need to fork 
or find another formatter. I speak as someone that maintains an internal fork 
with features that I do not deem worthy of foisting on trunk.
(And BTW @MyDeveloperDay, whoever you are, Thanks for all of your recent 
activity!)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55170/new/

https://reviews.llvm.org/D55170



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

[PATCH] D42034: [clang-format] In tests, expected code should be format-stable

[Mark Zeren via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL329231: [clang-format] In tests, expected code should be 
format-stable (authored by mzeren-vmw, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D42034

Files:
  cfe/trunk/unittests/Format/FormatTest.cpp
  cfe/trunk/unittests/Format/FormatTestComments.cpp
  cfe/trunk/unittests/Format/FormatTestJS.cpp
  cfe/trunk/unittests/Format/FormatTestJava.cpp
  cfe/trunk/unittests/Format/FormatTestObjC.cpp
  cfe/trunk/unittests/Format/FormatTestProto.cpp
  cfe/trunk/unittests/Format/FormatTestTextProto.cpp


Index: cfe/trunk/unittests/Format/FormatTestTextProto.cpp
===
--- cfe/trunk/unittests/Format/FormatTestTextProto.cpp
+++ cfe/trunk/unittests/Format/FormatTestTextProto.cpp
@@ -36,6 +36,7 @@
   }
 
   static void verifyFormat(llvm::StringRef Code, const FormatStyle ) {
+EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not 
stable";
 EXPECT_EQ(Code.str(), format(test::messUp(Code), Style));
   }
 
Index: cfe/trunk/unittests/Format/FormatTestProto.cpp
===
--- cfe/trunk/unittests/Format/FormatTestProto.cpp
+++ cfe/trunk/unittests/Format/FormatTestProto.cpp
@@ -38,6 +38,7 @@
   }
 
   static void verifyFormat(llvm::StringRef Code) {
+EXPECT_EQ(Code.str(), format(Code)) << "Expected code is not stable";
 EXPECT_EQ(Code.str(), format(test::messUp(Code)));
   }
 };
Index: cfe/trunk/unittests/Format/FormatTestComments.cpp
===
--- cfe/trunk/unittests/Format/FormatTestComments.cpp
+++ cfe/trunk/unittests/Format/FormatTestComments.cpp
@@ -70,6 +70,7 @@
 
   void verifyFormat(llvm::StringRef Code,
 const FormatStyle  = getLLVMStyle()) {
+EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not 
stable";
 EXPECT_EQ(Code.str(), format(test::messUp(Code), Style));
   }
 
Index: cfe/trunk/unittests/Format/FormatTestJava.cpp
===
--- cfe/trunk/unittests/Format/FormatTestJava.cpp
+++ cfe/trunk/unittests/Format/FormatTestJava.cpp
@@ -46,6 +46,7 @@
   static void verifyFormat(
   llvm::StringRef Code,
   const FormatStyle  = getGoogleStyle(FormatStyle::LK_Java)) {
+EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not 
stable";
 EXPECT_EQ(Code.str(), format(test::messUp(Code), Style));
   }
 };
Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@ -72,6 +72,8 @@
 
   void verifyFormat(llvm::StringRef Expected, llvm::StringRef Code,
 const FormatStyle  = getLLVMStyle()) {
+EXPECT_EQ(Expected.str(), format(Expected, Style))
+<< "Expected code is not stable";
 EXPECT_EQ(Expected.str(), format(Code, Style));
 if (Style.Language == FormatStyle::LK_Cpp) {
   // Objective-C++ is a superset of C++, so everything checked for C++
Index: cfe/trunk/unittests/Format/FormatTestObjC.cpp
===
--- cfe/trunk/unittests/Format/FormatTestObjC.cpp
+++ cfe/trunk/unittests/Format/FormatTestObjC.cpp
@@ -58,6 +58,7 @@
   }
 
   void verifyFormat(StringRef Code) {
+EXPECT_EQ(Code.str(), format(Code)) << "Expected code is not stable";
 EXPECT_EQ(Code.str(), format(test::messUp(Code)));
   }
 
Index: cfe/trunk/unittests/Format/FormatTestJS.cpp
===
--- cfe/trunk/unittests/Format/FormatTestJS.cpp
+++ cfe/trunk/unittests/Format/FormatTestJS.cpp
@@ -49,14 +49,18 @@
   static void verifyFormat(
   llvm::StringRef Code,
   const FormatStyle  = getGoogleStyle(FormatStyle::LK_JavaScript)) {
+EXPECT_EQ(Code.str(), format(Code, Style))
+<< "Expected code is not stable";
 std::string Result = format(test::messUp(Code), Style);
 EXPECT_EQ(Code.str(), Result) << "Formatted:\n" << Result;
   }
 
   static void verifyFormat(
   llvm::StringRef Expected,
   llvm::StringRef Code,
   const FormatStyle  = getGoogleStyle(FormatStyle::LK_JavaScript)) {
+EXPECT_EQ(Expected.str(), format(Expected, Style))
+<< "Expected code is not stable";
 std::string Result = format(Code, Style);
 EXPECT_EQ(Expected.str(), Result) << "Formatted:\n" << Result;
   }


Index: cfe/trunk/unittests/Format/FormatTestTextProto.cpp
===
--- cfe/trunk/unittests/Format/FormatTestTextProto.cpp
+++ cfe/trunk/unittests/Format/FormatTestTextProto.cpp
@@ -36,6 +36,7 @@
   }
 
   static void verifyFormat(llvm::StringRef Code, const FormatStyle ) {
+

[PATCH] D42034: [clang-format] In tests, expected code should be format-stable

[Mark Zeren via Phabricator via cfe-commits]

mzeren-vmw added a comment.

In https://reviews.llvm.org/D42034#1051965, @mzeren-vmw wrote:

> Update other verifyFormat implementations.


Ping?


Repository:
  rC Clang

https://reviews.llvm.org/D42034



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

[PATCH] D42034: [clang-format] In tests, expected code should be format-stable

[Mark Zeren via Phabricator via cfe-commits]

mzeren-vmw updated this revision to Diff 140276.
mzeren-vmw added a comment.

Update other verifyFormat implementations.


Repository:
  rC Clang

https://reviews.llvm.org/D42034

Files:
  unittests/Format/FormatTest.cpp
  unittests/Format/FormatTestComments.cpp
  unittests/Format/FormatTestJS.cpp
  unittests/Format/FormatTestJava.cpp
  unittests/Format/FormatTestObjC.cpp
  unittests/Format/FormatTestProto.cpp
  unittests/Format/FormatTestTextProto.cpp


Index: unittests/Format/FormatTestTextProto.cpp
===
--- unittests/Format/FormatTestTextProto.cpp
+++ unittests/Format/FormatTestTextProto.cpp
@@ -36,6 +36,7 @@
   }
 
   static void verifyFormat(llvm::StringRef Code, const FormatStyle ) {
+EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not 
stable";
 EXPECT_EQ(Code.str(), format(test::messUp(Code), Style));
   }
 
Index: unittests/Format/FormatTestProto.cpp
===
--- unittests/Format/FormatTestProto.cpp
+++ unittests/Format/FormatTestProto.cpp
@@ -38,6 +38,7 @@
   }
 
   static void verifyFormat(llvm::StringRef Code) {
+EXPECT_EQ(Code.str(), format(Code)) << "Expected code is not stable";
 EXPECT_EQ(Code.str(), format(test::messUp(Code)));
   }
 };
Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -58,6 +58,7 @@
   }
 
   void verifyFormat(StringRef Code) {
+EXPECT_EQ(Code.str(), format(Code)) << "Expected code is not stable";
 EXPECT_EQ(Code.str(), format(test::messUp(Code)));
   }
 
Index: unittests/Format/FormatTestJava.cpp
===
--- unittests/Format/FormatTestJava.cpp
+++ unittests/Format/FormatTestJava.cpp
@@ -46,6 +46,7 @@
   static void verifyFormat(
   llvm::StringRef Code,
   const FormatStyle  = getGoogleStyle(FormatStyle::LK_Java)) {
+EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not 
stable";
 EXPECT_EQ(Code.str(), format(test::messUp(Code), Style));
   }
 };
Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -49,14 +49,18 @@
   static void verifyFormat(
   llvm::StringRef Code,
   const FormatStyle  = getGoogleStyle(FormatStyle::LK_JavaScript)) {
+EXPECT_EQ(Code.str(), format(Code, Style))
+<< "Expected code is not stable";
 std::string Result = format(test::messUp(Code), Style);
 EXPECT_EQ(Code.str(), Result) << "Formatted:\n" << Result;
   }
 
   static void verifyFormat(
   llvm::StringRef Expected,
   llvm::StringRef Code,
   const FormatStyle  = getGoogleStyle(FormatStyle::LK_JavaScript)) {
+EXPECT_EQ(Expected.str(), format(Expected, Style))
+<< "Expected code is not stable";
 std::string Result = format(Code, Style);
 EXPECT_EQ(Expected.str(), Result) << "Formatted:\n" << Result;
   }
Index: unittests/Format/FormatTestComments.cpp
===
--- unittests/Format/FormatTestComments.cpp
+++ unittests/Format/FormatTestComments.cpp
@@ -70,6 +70,7 @@
 
   void verifyFormat(llvm::StringRef Code,
 const FormatStyle  = getLLVMStyle()) {
+EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not 
stable";
 EXPECT_EQ(Code.str(), format(test::messUp(Code), Style));
   }
 
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -72,6 +72,8 @@
 
   void verifyFormat(llvm::StringRef Expected, llvm::StringRef Code,
 const FormatStyle  = getLLVMStyle()) {
+EXPECT_EQ(Expected.str(), format(Expected, Style))
+<< "Expected code is not stable";
 EXPECT_EQ(Expected.str(), format(Code, Style));
 if (Style.Language == FormatStyle::LK_Cpp) {
   // Objective-C++ is a superset of C++, so everything checked for C++


Index: unittests/Format/FormatTestTextProto.cpp
===
--- unittests/Format/FormatTestTextProto.cpp
+++ unittests/Format/FormatTestTextProto.cpp
@@ -36,6 +36,7 @@
   }
 
   static void verifyFormat(llvm::StringRef Code, const FormatStyle ) {
+EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not stable";
 EXPECT_EQ(Code.str(), format(test::messUp(Code), Style));
   }
 
Index: unittests/Format/FormatTestProto.cpp
===
--- unittests/Format/FormatTestProto.cpp
+++ unittests/Format/FormatTestProto.cpp
@@ -38,6 +38,7 @@
   }
 
   static void verifyFormat(llvm::StringRef Code) {
+EXPECT_EQ(Code.str(), 

[PATCH] D42035: [clang-format] Fixup #include guard indents after parseFile()

[Mark Zeren via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL324238: [clang-format] Fixup #include guard indents after 
parseFile() (authored by mzeren-vmw, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D42035?vs=132394=132819#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D42035

Files:
  cfe/trunk/lib/Format/UnwrappedLineParser.cpp
  cfe/trunk/lib/Format/UnwrappedLineParser.h
  cfe/trunk/unittests/Format/FormatTest.cpp

Index: cfe/trunk/lib/Format/UnwrappedLineParser.h
===
--- cfe/trunk/lib/Format/UnwrappedLineParser.h
+++ cfe/trunk/lib/Format/UnwrappedLineParser.h
@@ -248,10 +248,23 @@
   // sequence.
   std::stack PPChainBranchIndex;
 
-  // Contains the #ifndef condition for a potential include guard.
-  FormatToken *IfNdefCondition;
-  bool FoundIncludeGuardStart;
-  bool IncludeGuardRejected;
+  // Include guard search state. Used to fixup preprocessor indent levels
+  // so that include guards do not participate in indentation.
+  enum IncludeGuardState {
+IG_Inited,
+IG_IfNdefed,
+IG_Defined,
+IG_Found,
+IG_Rejected,
+  };
+
+  // Current state of include guard search.
+  IncludeGuardState IncludeGuard;
+
+  // Points to the #ifndef condition for a potential include guard. Null unless
+  // IncludeGuardState == IG_IfNdefed.
+  FormatToken *IncludeGuardToken;
+
   // Contains the first start column where the source begins. This is zero for
   // normal source code and may be nonzero when formatting a code fragment that
   // does not start at the beginning of the file.
Index: cfe/trunk/lib/Format/UnwrappedLineParser.cpp
===
--- cfe/trunk/lib/Format/UnwrappedLineParser.cpp
+++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp
@@ -234,14 +234,15 @@
   CurrentLines(), Style(Style), Keywords(Keywords),
   CommentPragmasRegex(Style.CommentPragmas), Tokens(nullptr),
   Callback(Callback), AllTokens(Tokens), PPBranchLevel(-1),
-  IfNdefCondition(nullptr), FoundIncludeGuardStart(false),
-  IncludeGuardRejected(false), FirstStartColumn(FirstStartColumn) {}
+  IncludeGuard(Style.IndentPPDirectives == FormatStyle::PPDIS_None
+   ? IG_Rejected
+   : IG_Inited),
+  IncludeGuardToken(nullptr), FirstStartColumn(FirstStartColumn) {}
 
 void UnwrappedLineParser::reset() {
   PPBranchLevel = -1;
-  IfNdefCondition = nullptr;
-  FoundIncludeGuardStart = false;
-  IncludeGuardRejected = false;
+  IncludeGuard = IG_Inited;
+  IncludeGuardToken = nullptr;
   Line.reset(new UnwrappedLine);
   CommentsBeforeNextToken.clear();
   FormatTok = nullptr;
@@ -264,6 +265,14 @@
 
 readToken();
 parseFile();
+
+// If we found an include guard then all preprocessor directives (other than
+// the guard) are over-indented by one.
+if (IncludeGuard == IG_Found)
+  for (auto  : Lines)
+if (Line.InPPDirective && Line.Level > 0)
+  --Line.Level;
+
 // Create line with eof token.
 pushToken(FormatTok);
 addUnwrappedLine();
@@ -724,26 +733,28 @@
   // 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) {
+  if (IncludeGuard == IG_Inited && MaybeIncludeGuard) {
 for (auto  : Lines) {
   if (!Line.Tokens.front().Tok->is(tok::comment)) {
 MaybeIncludeGuard = false;
-IncludeGuardRejected = true;
+IncludeGuard = IG_Rejected;
 break;
   }
 }
   }
   --PPBranchLevel;
   parsePPUnknown();
   ++PPBranchLevel;
-  if (!IncludeGuardRejected && !FoundIncludeGuardStart && MaybeIncludeGuard)
-IfNdefCondition = IfCondition;
+  if (IncludeGuard == IG_Inited && MaybeIncludeGuard) {
+IncludeGuard = IG_IfNdefed;
+IncludeGuardToken = IfCondition;
+  }
 }
 
 void UnwrappedLineParser::parsePPElse() {
   // If a potential include guard has an #else, it's not an include guard.
-  if (FoundIncludeGuardStart && PPBranchLevel == 0)
-FoundIncludeGuardStart = false;
+  if (IncludeGuard == IG_Defined && PPBranchLevel == 0)
+IncludeGuard = IG_Rejected;
   conditionalCompilationAlternative();
   if (PPBranchLevel > -1)
 --PPBranchLevel;
@@ -757,34 +768,37 @@
   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.
+  // then we found an include guard.
   unsigned TokenPosition = Tokens->getPosition();
   FormatToken *PeekNext = AllTokens[TokenPosition];
-  if (FoundIncludeGuardStart && PPBranchLevel == -1 && PeekNext->is(tok::eof) &&
+  if (IncludeGuard == IG_Defined && PPBranchLevel == -1 

[PATCH] D42034: [clang-format] In tests, expected code should be format-stable

[Mark Zeren via Phabricator via cfe-commits]

mzeren-vmw updated this revision to Diff 132395.
mzeren-vmw added a comment.

- Reviewers: drop euhlmann, add djasper
- Rebase
- Ping


Repository:
  rC Clang

https://reviews.llvm.org/D42034

Files:
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -72,6 +72,7 @@
 
   void verifyFormat(llvm::StringRef Expected, llvm::StringRef Code,
 const FormatStyle  = getLLVMStyle()) {
+EXPECT_EQ(Expected.str(), format(Expected, Style));
 EXPECT_EQ(Expected.str(), format(Code, Style));
 if (Style.Language == FormatStyle::LK_Cpp) {
   // Objective-C++ is a superset of C++, so everything checked for C++


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -72,6 +72,7 @@
 
   void verifyFormat(llvm::StringRef Expected, llvm::StringRef Code,
 const FormatStyle  = getLLVMStyle()) {
+EXPECT_EQ(Expected.str(), format(Expected, Style));
 EXPECT_EQ(Expected.str(), format(Code, Style));
 if (Style.Language == FormatStyle::LK_Cpp) {
   // Objective-C++ is a superset of C++, so everything checked for C++
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42035: [clang-format] Fixup #include guard indents after parseFile()

[Mark Zeren via Phabricator via cfe-commits]

mzeren-vmw updated this revision to Diff 132394.
mzeren-vmw added a comment.

- Add comments to IncludeGuardState.
- Fix re-initialization of IncludeGuard in UnwrappedLineParser::reset.
- Remove unnecessary block after if.
- Rebase


Repository:
  rC Clang

https://reviews.llvm.org/D42035

Files:
  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
@@ -2547,6 +2547,20 @@
"#elif FOO\n"
"#endif",
Style);
+  // Non-identifier #define after potential include guard.
+  verifyFormat("#ifndef FOO\n"
+   "#  define 1\n"
+   "#endif\n",
+   Style);
+  // #if closes past last non-preprocessor line.
+  verifyFormat("#ifndef FOO\n"
+   "#define FOO\n"
+   "#if 1\n"
+   "int i;\n"
+   "#  define A 0\n"
+   "#endif\n"
+   "#endif\n",
+   Style);
   // FIXME: This doesn't handle 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
Index: lib/Format/UnwrappedLineParser.h
===
--- lib/Format/UnwrappedLineParser.h
+++ lib/Format/UnwrappedLineParser.h
@@ -248,10 +248,23 @@
   // sequence.
   std::stack PPChainBranchIndex;
 
-  // Contains the #ifndef condition for a potential include guard.
-  FormatToken *IfNdefCondition;
-  bool FoundIncludeGuardStart;
-  bool IncludeGuardRejected;
+  // Include guard search state. Used to fixup preprocessor indent levels
+  // so that include guards do not participate in indentation.
+  enum IncludeGuardState {
+IG_Inited,   // Search started, looking for #ifndef.
+IG_IfNdefed, // #ifndef found, IncludeGuardToken points to condition.
+IG_Defined,  // Matching #define found, checking other requirements.
+IG_Found,// All requirements met, need to fix indents.
+IG_Rejected, // Search failed or never started.
+  };
+
+  // Current state of include guard search.
+  IncludeGuardState IncludeGuard;
+
+  // Points to the #ifndef condition for a potential include guard. Null unless
+  // IncludeGuardState == IG_IfNdefed.
+  FormatToken *IncludeGuardToken;
+
   // Contains the first start column where the source begins. This is zero for
   // normal source code and may be nonzero when formatting a code fragment that
   // does not start at the beginning of the file.
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -234,14 +234,17 @@
   CurrentLines(), Style(Style), Keywords(Keywords),
   CommentPragmasRegex(Style.CommentPragmas), Tokens(nullptr),
   Callback(Callback), AllTokens(Tokens), PPBranchLevel(-1),
-  IfNdefCondition(nullptr), FoundIncludeGuardStart(false),
-  IncludeGuardRejected(false), FirstStartColumn(FirstStartColumn) {}
+  IncludeGuard(Style.IndentPPDirectives == FormatStyle::PPDIS_None
+   ? IG_Rejected
+   : IG_Inited),
+  IncludeGuardToken(nullptr), FirstStartColumn(FirstStartColumn) {}
 
 void UnwrappedLineParser::reset() {
   PPBranchLevel = -1;
-  IfNdefCondition = nullptr;
-  FoundIncludeGuardStart = false;
-  IncludeGuardRejected = false;
+  IncludeGuard = Style.IndentPPDirectives == FormatStyle::PPDIS_None
+ ? IG_Rejected
+ : IG_Inited;
+  IncludeGuardToken = nullptr;
   Line.reset(new UnwrappedLine);
   CommentsBeforeNextToken.clear();
   FormatTok = nullptr;
@@ -264,6 +267,14 @@
 
 readToken();
 parseFile();
+
+// If we found an include guard then all preprocessor directives (other than
+// the guard) are over-indented by one.
+if (IncludeGuard == IG_Found)
+  for (auto  : Lines)
+if (Line.InPPDirective && Line.Level > 0)
+  --Line.Level;
+
 // Create line with eof token.
 pushToken(FormatTok);
 addUnwrappedLine();
@@ -724,26 +735,27 @@
   // 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) {
+  if (IncludeGuard == IG_Inited && MaybeIncludeGuard)
 for (auto  : Lines) {
   if (!Line.Tokens.front().Tok->is(tok::comment)) {
 MaybeIncludeGuard = false;
-IncludeGuardRejected = true;
+IncludeGuard = IG_Rejected;
 break;
   }
 }
-  }
   --PPBranchLevel;
   parsePPUnknown();
   ++PPBranchLevel;
-  if (!IncludeGuardRejected && 

[PATCH] D42035: [clang-format] Fixup #include guard indents after parseFile()

[Mark Zeren via Phabricator via cfe-commits]

mzeren-vmw added inline comments.



Comment at: lib/Format/UnwrappedLineParser.cpp:244
   PPBranchLevel = -1;
-  IfNdefCondition = nullptr;
-  FoundIncludeGuardStart = false;
-  IncludeGuardRejected = false;
+  IncludeGuard = IG_Inited;
+  IncludeGuardToken = nullptr;

Hm. From self review, I think this should be:

IncludeGuard = Style.IndentPPDirectives == FormatStyle::PPDIS_None ? 
IG_Rejected : IG_Inited;



Comment at: lib/Format/UnwrappedLineParser.cpp:736
   bool MaybeIncludeGuard = IfNDef;
-  if (!IncludeGuardRejected && !FoundIncludeGuardStart && MaybeIncludeGuard) {
+  if (IncludeGuard == IG_Inited && MaybeIncludeGuard) {
 for (auto  : Lines) {

technically I could drop the braces opened on this line. Would you like me to 
do that?


Repository:
  rC Clang

https://reviews.llvm.org/D42035



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

[PATCH] D42035: [clang-format] Fixup #include guard indents after parseFile()

[Mark Zeren via Phabricator via cfe-commits]

mzeren-vmw updated this revision to Diff 132320.
mzeren-vmw added a comment.

rebase, ping.


Repository:
  rC Clang

https://reviews.llvm.org/D42035

Files:
  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
@@ -2548,6 +2548,20 @@
"#elif FOO\n"
"#endif",
Style);
+  // Non-identifier #define after potential include guard.
+  verifyFormat("#ifndef FOO\n"
+   "#  define 1\n"
+   "#endif\n",
+   Style);
+  // #if closes past last non-preprocessor line.
+  verifyFormat("#ifndef FOO\n"
+   "#define FOO\n"
+   "#if 1\n"
+   "int i;\n"
+   "#  define A 0\n"
+   "#endif\n"
+   "#endif\n",
+   Style);
   // FIXME: This doesn't handle 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
Index: lib/Format/UnwrappedLineParser.h
===
--- lib/Format/UnwrappedLineParser.h
+++ lib/Format/UnwrappedLineParser.h
@@ -248,10 +248,23 @@
   // sequence.
   std::stack PPChainBranchIndex;
 
-  // Contains the #ifndef condition for a potential include guard.
-  FormatToken *IfNdefCondition;
-  bool FoundIncludeGuardStart;
-  bool IncludeGuardRejected;
+  // Include guard search state. Used to fixup preprocessor indent levels
+  // so that include guards do not participate in indentation.
+  enum IncludeGuardState {
+IG_Inited,
+IG_IfNdefed,
+IG_Defined,
+IG_Found,
+IG_Rejected,
+  };
+
+  // Current state of include guard search.
+  IncludeGuardState IncludeGuard;
+
+  // Points to the #ifndef condition for a potential include guard. Null unless
+  // IncludeGuardState == IG_IfNdefed.
+  FormatToken *IncludeGuardToken;
+
   // Contains the first start column where the source begins. This is zero for
   // normal source code and may be nonzero when formatting a code fragment that
   // does not start at the beginning of the file.
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -234,14 +234,15 @@
   CurrentLines(), Style(Style), Keywords(Keywords),
   CommentPragmasRegex(Style.CommentPragmas), Tokens(nullptr),
   Callback(Callback), AllTokens(Tokens), PPBranchLevel(-1),
-  IfNdefCondition(nullptr), FoundIncludeGuardStart(false),
-  IncludeGuardRejected(false), FirstStartColumn(FirstStartColumn) {}
+  IncludeGuard(Style.IndentPPDirectives == FormatStyle::PPDIS_None
+   ? IG_Rejected
+   : IG_Inited),
+  IncludeGuardToken(nullptr), FirstStartColumn(FirstStartColumn) {}
 
 void UnwrappedLineParser::reset() {
   PPBranchLevel = -1;
-  IfNdefCondition = nullptr;
-  FoundIncludeGuardStart = false;
-  IncludeGuardRejected = false;
+  IncludeGuard = IG_Inited;
+  IncludeGuardToken = nullptr;
   Line.reset(new UnwrappedLine);
   CommentsBeforeNextToken.clear();
   FormatTok = nullptr;
@@ -264,6 +265,14 @@
 
 readToken();
 parseFile();
+
+// If we found an include guard then all preprocessor directives (other than
+// the guard) are over-indented by one.
+if (IncludeGuard == IG_Found)
+  for (auto  : Lines)
+if (Line.InPPDirective && Line.Level > 0)
+  --Line.Level;
+
 // Create line with eof token.
 pushToken(FormatTok);
 addUnwrappedLine();
@@ -724,26 +733,28 @@
   // 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) {
+  if (IncludeGuard == IG_Inited && MaybeIncludeGuard) {
 for (auto  : Lines) {
   if (!Line.Tokens.front().Tok->is(tok::comment)) {
 MaybeIncludeGuard = false;
-IncludeGuardRejected = true;
+IncludeGuard = IG_Rejected;
 break;
   }
 }
   }
   --PPBranchLevel;
   parsePPUnknown();
   ++PPBranchLevel;
-  if (!IncludeGuardRejected && !FoundIncludeGuardStart && MaybeIncludeGuard)
-IfNdefCondition = IfCondition;
+  if (IncludeGuard == IG_Inited && MaybeIncludeGuard) {
+IncludeGuard = IG_IfNdefed;
+IncludeGuardToken = IfCondition;
+  }
 }
 
 void UnwrappedLineParser::parsePPElse() {
   // If a potential include guard has an #else, it's not an include guard.
-  if (FoundIncludeGuardStart && PPBranchLevel == 0)
-FoundIncludeGuardStart = false;
+  if (IncludeGuard == IG_Defined && PPBranchLevel == 0)
+

[PATCH] D42408: [clang-format] Align preprocessor comments with #

[Mark Zeren via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL323904: [clang-format] Align preprocessor comments with # 
(authored by mzeren-vmw, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D42408?vs=131003=132245#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D42408

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

Index: cfe/trunk/lib/Format/TokenAnnotator.cpp
===
--- cfe/trunk/lib/Format/TokenAnnotator.cpp
+++ cfe/trunk/lib/Format/TokenAnnotator.cpp
@@ -1725,15 +1725,18 @@
   }
 }
 
-if (NextNonCommentLine && CommentLine) {
-  // If the comment is currently aligned with the line immediately following
-  // it, that's probably intentional and we should keep it.
-  bool AlignedWithNextLine =
-  NextNonCommentLine->First->NewlinesBefore <= 1 &&
-  NextNonCommentLine->First->OriginalColumn ==
-  (*I)->First->OriginalColumn;
-  if (AlignedWithNextLine)
-(*I)->Level = NextNonCommentLine->Level;
+// If the comment is currently aligned with the line immediately following
+// it, that's probably intentional and we should keep it.
+if (NextNonCommentLine && CommentLine &&
+NextNonCommentLine->First->NewlinesBefore <= 1 &&
+NextNonCommentLine->First->OriginalColumn ==
+(*I)->First->OriginalColumn) {
+  // Align comments for preprocessor lines with the # in column 0.
+  // Otherwise, align with the next line.
+  (*I)->Level = (NextNonCommentLine->Type == LT_PreprocessorDirective ||
+ NextNonCommentLine->Type == LT_ImportStatement)
+? 0
+: NextNonCommentLine->Level;
 } else {
   NextNonCommentLine = (*I)->First->isNot(tok::r_brace) ? (*I) : nullptr;
 }
Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@ -2595,21 +2595,85 @@
"code();\n"
"#endif",
Style));
-  // FIXME: The comment indent corrector in TokenAnnotator gets thrown off by
-  // preprocessor indentation.
-  EXPECT_EQ("#if 1\n"
-"  // comment\n"
-"#  define A 0\n"
-"// comment\n"
-"#  define B 0\n"
-"#endif",
-format("#if 1\n"
-   "// comment\n"
-   "#  define A 0\n"
-   "   // comment\n"
-   "#  define B 0\n"
-   "#endif",
-   Style));
+  // Keep comments aligned with #, otherwise indent comments normally. These
+  // tests cannot use verifyFormat because messUp manipulates leading
+  // whitespace.
+  {
+const char *Expected = ""
+   "void f() {\n"
+   "#if 1\n"
+   "// Preprocessor aligned.\n"
+   "#  define A 0\n"
+   "  // Code. Separated by blank line.\n"
+   "\n"
+   "#  define B 0\n"
+   "  // Code. Not aligned with #\n"
+   "#  define C 0\n"
+   "#endif";
+const char *ToFormat = ""
+   "void f() {\n"
+   "#if 1\n"
+   "// Preprocessor aligned.\n"
+   "#  define A 0\n"
+   "// Code. Separated by blank line.\n"
+   "\n"
+   "#  define B 0\n"
+   "   // Code. Not aligned with #\n"
+   "#  define C 0\n"
+   "#endif";
+EXPECT_EQ(Expected, format(ToFormat, Style));
+EXPECT_EQ(Expected, format(Expected, Style));
+  }
+  // Keep block quotes aligned.
+  {
+const char *Expected = ""
+   "void f() {\n"
+   "#if 1\n"
+   "/* Preprocessor aligned. */\n"
+   "#  define A 0\n"
+   "  /* Code. Separated by blank line. */\n"
+   "\n"
+   "#  define B 0\n"
+   "  /* Code. Not aligned with # */\n"
+   "#  define C 0\n"
+   "#endif";
+const char *ToFormat = ""
+   "void f() {\n"
+   "#if 1\n"
+   "/* Preprocessor aligned. */\n"
+   "#  define A 0\n"
+   "/* Code. Separated by blank line. */\n"
+   "\n"
+ 

[PATCH] D42408: [clang-format] Align preprocessor comments with #

[Mark Zeren via Phabricator via cfe-commits]

mzeren-vmw created this revision.
mzeren-vmw added reviewers: krasimir, klimek, djasper.

r312125, which introduced preprocessor indentation, shipped with a known
issue where "indentation of comments immediately before indented
preprocessor lines is toggled on each run". For example these two forms
toggle:

  #ifndef HEADER_H
  #define HEADER_H
  #if 1
  // comment
  #   define A 0
  #endif
  #endif
  
  #ifndef HEADER_H
  #define HEADER_H
  #if 1
 // comment
  #   define A 0
  #endif
  #endif

This happens because we check vertical alignment against the '#' yet
indent to the level of the 'define'. This patch resolves this issue by
aligning against the '#'.


Repository:
  rC Clang

https://reviews.llvm.org/D42408

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

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2610,21 +2610,85 @@
"code();\n"
"#endif",
Style));
-  // FIXME: The comment indent corrector in TokenAnnotator gets thrown off by
-  // preprocessor indentation.
-  EXPECT_EQ("#if 1\n"
-"  // comment\n"
-"#  define A 0\n"
-"// comment\n"
-"#  define B 0\n"
-"#endif",
-format("#if 1\n"
-   "// comment\n"
-   "#  define A 0\n"
-   "   // comment\n"
-   "#  define B 0\n"
-   "#endif",
-   Style));
+  // Keep comments aligned with #, otherwise indnet comments normally. These
+  // tests cannot use verifyFormat because messUp manipulates leading
+  // whitespace.
+  {
+const char *Expected = ""
+   "void f() {\n"
+   "#if 1\n"
+   "// Preprocessor aligned.\n"
+   "#  define A 0\n"
+   "  // Code. Separated by blank line.\n"
+   "\n"
+   "#  define B 0\n"
+   "  // Code. Not aligned with #\n"
+   "#  define C 0\n"
+   "#endif";
+const char *ToFormat = ""
+   "void f() {\n"
+   "#if 1\n"
+   "// Preprocessor aligned.\n"
+   "#  define A 0\n"
+   "// Code. Separated by blank line.\n"
+   "\n"
+   "#  define B 0\n"
+   "   // Code. Not aligned with #\n"
+   "#  define C 0\n"
+   "#endif";
+EXPECT_EQ(Expected, format(ToFormat, Style));
+EXPECT_EQ(Expected, format(Expected, Style));
+  }
+  // Keep block quotes aligned.
+  {
+const char *Expected = ""
+   "void f() {\n"
+   "#if 1\n"
+   "/* Preprocessor aligned. */\n"
+   "#  define A 0\n"
+   "  /* Code. Separated by blank line. */\n"
+   "\n"
+   "#  define B 0\n"
+   "  /* Code. Not aligned with # */\n"
+   "#  define C 0\n"
+   "#endif";
+const char *ToFormat = ""
+   "void f() {\n"
+   "#if 1\n"
+   "/* Preprocessor aligned. */\n"
+   "#  define A 0\n"
+   "/* Code. Separated by blank line. */\n"
+   "\n"
+   "#  define B 0\n"
+   "   /* Code. Not aligned with # */\n"
+   "#  define C 0\n"
+   "#endif";
+EXPECT_EQ(Expected, format(ToFormat, Style));
+EXPECT_EQ(Expected, format(Expected, Style));
+  }
+  // Keep comments aligned with un-indented directives.
+  {
+const char *Expected = ""
+   "void f() {\n"
+   "// Preprocessor aligned.\n"
+   "#define A 0\n"
+   "  // Code. Separated by blank line.\n"
+   "\n"
+   "#define B 0\n"
+   "  // Code. Not aligned with #\n"
+   "#define C 0\n";
+const char *ToFormat = ""
+   "void f() {\n"
+   "// Preprocessor aligned.\n"
+   "#define A 0\n"
+   "// Code. Separated by blank line.\n"
+   "\n"
+   "#define B 0\n"
+   "   // Code. Not aligned with #\n"
+   "#define C 0\n";

[PATCH] D42036: [clang-format] Keep comments aligned to macros

[Mark Zeren via Phabricator via cfe-commits]

mzeren-vmw added a comment.

In https://reviews.llvm.org/D42036#984401, @djasper wrote:

> To me, aligning with the define seems fundamentally wrong.


we definitely have code that does that internally. It can also be seen in the 
wild e.g.:
https://github.com/boostorg/config/blob/develop/include/boost/config/detail/posix_features.hpp
However, it seems reasonable that clang-format's "default" be alignment with #. 
That will be a simpler patch, and it will resolve the toggling behavior. Let me 
work that up in a separate review.


Repository:
  rC Clang

https://reviews.llvm.org/D42036



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

[PATCH] D42036: [clang-format] Keep comments aligned to macros

[Mark Zeren via Phabricator via cfe-commits]

mzeren-vmw marked 2 inline comments as done.
mzeren-vmw added inline comments.



Comment at: lib/Format/TokenAnnotator.cpp:1756
+  if (Alignment == CA_Preprocessor)
+(*I)->LevelOffset = 1;
 } else {

krasimir wrote:
> This feels a bit awkward: we're adding code that implicitly assumes the exact 
> style the preprocessor directives and comments around them are handled. Maybe 
> if this could become part of the level itself, it would feel less awkward.
I agree that the "long distance coupling" is awkward. Perhaps the new 
enumerator names make this a bit more palatable?



Comment at: lib/Format/TokenAnnotator.h:41
   AnnotatedLine(const UnwrappedLine )
-  : First(Line.Tokens.front().Tok), Level(Line.Level),
+  : First(Line.Tokens.front().Tok), Level(Line.Level), LevelOffset(0),
 MatchingOpeningBlockLineIndex(Line.MatchingOpeningBlockLineIndex),

krasimir wrote:
> Is there a way to not introduce `LevelOffset`, but have it part of `Level`?
`Level` is an abstract indentation level, while `LevelOffset` is "columns". 
They have different units. Maybe it would be feasible to change the units of 
"Level" to columns in order to merge these two variables, but doing so would 
throw away information. It also seems like a much larger change. We could 
create a composite type `class AnnotatedLevel { private: unsigned Level, 
unsigned Offset public:   }` but that seems 
over-engineered. Any other ideas?


Repository:
  rC Clang

https://reviews.llvm.org/D42036



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

[PATCH] D42036: [clang-format] Keep comments aligned to macros

[Mark Zeren via Phabricator via cfe-commits]

mzeren-vmw updated this revision to Diff 130487.
mzeren-vmw added a comment.

Documented CommentAlignment enumerators. Documenting them suggested better
enumerator names.

Added tests for multi-line comments, block comments and trailing comments.


Repository:
  rC Clang

https://reviews.llvm.org/D42036

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

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2497,7 +2497,6 @@
"code();\n"
"#endif",
Style);
-
   // Include guards must cover the entire file.
   verifyFormat("code();\n"
"code();\n"
@@ -2593,21 +2592,97 @@
"code();\n"
"#endif",
Style));
-  // FIXME: The comment indent corrector in TokenAnnotator gets thrown off by
-  // preprocessor indentation.
-  EXPECT_EQ("#if 1\n"
-"  // comment\n"
-"#  define A 0\n"
-"// comment\n"
-"#  define B 0\n"
-"#endif",
-format("#if 1\n"
-   "// comment\n"
-   "#  define A 0\n"
-   "   // comment\n"
-   "#  define B 0\n"
-   "#endif",
-   Style));
+  // Comments aligned to macros stay aligned. This test is incompatible with
+  // verifyFormat() because messUp() removes the alignment.
+  {
+const char *Expected = ""
+   "// Level 0 unaligned comment\n"
+   "// Level 0 unaligned continuation\n"
+   "#ifndef HEADER_H\n"
+   "// Level 0 aligned comment\n"
+   "// Level 0 aligned continuation\n"
+   "#define HEADER_H\n"
+   "#if 1\n"
+   "   // aligned comment\n"
+   "   // aligned continuation\n"
+   "#  define A 0\n"
+   "// un-aligned comment\n"
+   "// un-aligned continuation\n"
+   "#  define B 0\n"
+   "// Trailing aligned comment\n"
+   "#endif\n"
+   "#endif";
+const char *ToFormat = ""
+   " // Level 0 unaligned comment\n"
+   " // Level 0 unaligned continuation\n"
+   "#ifndef HEADER_H\n"
+   "// Level 0 aligned comment\n"
+   "// Level 0 aligned continuation\n"
+   "#define HEADER_H\n"
+   "#if 1\n"
+   "   // aligned comment\n"
+   "   // aligned continuation\n"
+   "#  define A 0\n"
+   "  // un-aligned comment\n"
+   "  // un-aligned continuation\n"
+   "#  define B 0\n"
+   "   // Trailing aligned comment\n"
+   "#endif\n"
+   "#endif";
+EXPECT_EQ(Expected, format(ToFormat, Style));
+EXPECT_EQ(Expected, format(Expected, Style));
+  }
+  // Same as above, but block comments.
+  {
+const char *Expected = ""
+   "/*\n"
+   " * Level 0 unaligned block comment\n"
+   " */\n"
+   "#ifndef HEADER_H\n"
+   "/*\n"
+   " *  Level 0 aligned comment\n"
+   " */\n"
+   "#define HEADER_H\n"
+   "#if 1\n"
+   "   /*\n"
+   "* Aligned comment\n"
+   "*/\n"
+   "#  define A 0\n"
+   "/*\n"
+   " * Unaligned comment\n"
+   " */\n"
+   "#  define B 0\n"
+   "/*\n"
+   " * Trailing aligned comment\n"
+   " */\n"
+   "#endif\n"
+   "#endif";
+const char *ToFormat = ""
+   " /*\n"
+   "  * Level 0 unaligned block comment\n"
+   "  */\n"
+   "#ifndef HEADER_H\n"
+   "/*\n"
+   " *  Level 0 aligned comment\n"
+   " */\n"
+   "#define HEADER_H\n"
+   "#if 1\n"
+   "   /*\n"
+  

[PATCH] D42036: [clang-format] Keep comments aligned to macros

[Mark Zeren via Phabricator via cfe-commits]

mzeren-vmw created this revision.
mzeren-vmw added reviewers: euhlmann, krasimir, klimek.
Herald added a subscriber: cfe-commits.

r312125, which introduced preprocessor indentation, shipped with a known
issue where "indentation of comments immediately before indented
preprocessor lines is toggled on each run". For example these two forms
toggle:

  #ifndef HEADER_H
  #define HEADER_H
  #if 1
  // comment
  #   define A 0
  #endif
  #endif
  
  #ifndef HEADER_H
  #define HEADER_H
  #if 1
 // comment
  #   define A 0
  #endif
  #endif

This happens because we check vertical alignment against the "#" yet
indent to the level of the "define". This patch resolves this issue by
checking vertical alignment against the "define", and by tracking a
"LevelOffset" (0 or 1) in each AnnotatedLine to account for the
off-by-one indentation of preprocessor lines.


Repository:
  rC Clang

https://reviews.llvm.org/D42036

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

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2497,7 +2497,6 @@
"code();\n"
"#endif",
Style);
-
   // Include guards must cover the entire file.
   verifyFormat("code();\n"
"code();\n"
@@ -2593,21 +2592,34 @@
"code();\n"
"#endif",
Style));
-  // FIXME: The comment indent corrector in TokenAnnotator gets thrown off by
-  // preprocessor indentation.
-  EXPECT_EQ("#if 1\n"
-"  // comment\n"
-"#  define A 0\n"
-"// comment\n"
-"#  define B 0\n"
-"#endif",
-format("#if 1\n"
-   "// comment\n"
-   "#  define A 0\n"
-   "   // comment\n"
-   "#  define B 0\n"
-   "#endif",
-   Style));
+  // Comments aligned to macros stay aligned. This test is incompatible with
+  // verifyFormat() because messUp() removes the alignment.
+  {
+const char *Expected = "// Level 0 unaligned comment\n"
+   "#ifndef HEADER_H\n"
+   "// Level 0 aligned comment\n"
+   "#define HEADER_H\n"
+   "#if 1\n"
+   "   // aligned comment\n"
+   "#  define A 0\n"
+   "// un-aligned comment\n"
+   "#  define B 0\n"
+   "#endif\n"
+   "#endif";
+const char *ToFormat = " // Level 0 unaligned comment\n"
+   "#ifndef HEADER_H\n"
+   "// Level 0 aligned comment\n"
+   "#define HEADER_H\n"
+   "#if 1\n"
+   "   // aligned comment\n"
+   "#  define A 0\n"
+   "  // un-aligned comment\n"
+   "#  define B 0\n"
+   "#endif\n"
+   "#endif";
+EXPECT_EQ(Expected, format(ToFormat, Style));
+EXPECT_EQ(Expected, format(Expected, Style));
+  }
   // Test with tabs.
   Style.UseTab = FormatStyle::UT_Always;
   Style.IndentWidth = 8;
Index: lib/Format/UnwrappedLineFormatter.cpp
===
--- lib/Format/UnwrappedLineFormatter.cpp
+++ lib/Format/UnwrappedLineFormatter.cpp
@@ -52,6 +52,7 @@
   /// next.
   void nextLine(const AnnotatedLine ) {
 Offset = getIndentOffset(*Line.First);
+Offset += Line.LevelOffset;
 // Update the indent level cache size so that we can rely on it
 // having the right size in adjustToUnmodifiedline.
 while (IndentForLevel.size() <= Line.Level)
Index: lib/Format/TokenAnnotator.h
===
--- lib/Format/TokenAnnotator.h
+++ lib/Format/TokenAnnotator.h
@@ -38,7 +38,7 @@
 class AnnotatedLine {
 public:
   AnnotatedLine(const UnwrappedLine )
-  : First(Line.Tokens.front().Tok), Level(Line.Level),
+  : First(Line.Tokens.front().Tok), Level(Line.Level), LevelOffset(0),
 MatchingOpeningBlockLineIndex(Line.MatchingOpeningBlockLineIndex),
 InPPDirective(Line.InPPDirective),
 MustBeDeclaration(Line.MustBeDeclaration), MightBeFunctionDecl(false),
@@ -111,6 +111,12 @@
 
   LineType Type;
   unsigned Level;
+
+  /// Adjustment to Level based indent. When comments are aligned to the next
+  /// preprocessor line they must use the same offset as the directive,
+  /// typically 1 due to the leading #.
+  unsigned LevelOffset;
+
   size_t MatchingOpeningBlockLineIndex;
   bool InPPDirective;
   bool MustBeDeclaration;
Index: 

[PATCH] D42035: [clang-format] Fixup #include guard indents after parseFile()

[Mark Zeren via Phabricator via cfe-commits]

mzeren-vmw created this revision.
mzeren-vmw added reviewers: euhlmann, krasimir, klimek.
Herald added a subscriber: cfe-commits.

When a preprocessor indent closes after the last line of normal code we do not
correctly fixup include guard indents. For example:

  #ifndef HEADER_H
  #define HEADER_H
  #if 1
  int i;
  #  define A 0
  #endif
  #endif

incorrectly reformats to:

  #ifndef HEADER_H
  #define HEADER_H
  #if 1
  int i;
  #define A 0
  #  endif
  #endif

To resolve this issue we must fixup levels after parseFile(). Delaying
the fixup introduces a new state, so consolidate include guard search
state into an enum.


Repository:
  rC Clang

https://reviews.llvm.org/D42035

Files:
  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
@@ -2531,6 +2531,20 @@
"#elif FOO\n"
"#endif",
Style);
+  // Non-identifier #define after potential include guard.
+  verifyFormat("#ifndef FOO\n"
+   "#  define 1\n"
+   "#endif\n",
+   Style);
+  // #if closes past last non-preprocessor line.
+  verifyFormat("#ifndef FOO\n"
+   "#define FOO\n"
+   "#if 1\n"
+   "int i;\n"
+   "#  define A 0\n"
+   "#endif\n"
+   "#endif\n",
+   Style);
   // FIXME: This doesn't handle 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
Index: lib/Format/UnwrappedLineParser.h
===
--- lib/Format/UnwrappedLineParser.h
+++ lib/Format/UnwrappedLineParser.h
@@ -248,10 +248,23 @@
   // sequence.
   std::stack PPChainBranchIndex;
 
-  // Contains the #ifndef condition for a potential include guard.
-  FormatToken *IfNdefCondition;
-  bool FoundIncludeGuardStart;
-  bool IncludeGuardRejected;
+  // Include guard search state. Used to fixup preprocessor indent levels
+  // so that include guards do not participate in indentation.
+  enum IncludeGuardState {
+IG_Inited,
+IG_IfNdefed,
+IG_Defined,
+IG_Found,
+IG_Rejected,
+  };
+
+  // Current state of include guard search.
+  IncludeGuardState IncludeGuard;
+
+  // Points to the #ifndef condition for a potential include guard. Null unless
+  // IncludeGuardState == IG_IfNdefed.
+  FormatToken *IncludeGuardToken;
+
   // Contains the first start column where the source begins. This is zero for
   // normal source code and may be nonzero when formatting a code fragment that
   // does not start at the beginning of the file.
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -234,14 +234,15 @@
   CurrentLines(), Style(Style), Keywords(Keywords),
   CommentPragmasRegex(Style.CommentPragmas), Tokens(nullptr),
   Callback(Callback), AllTokens(Tokens), PPBranchLevel(-1),
-  IfNdefCondition(nullptr), FoundIncludeGuardStart(false),
-  IncludeGuardRejected(false), FirstStartColumn(FirstStartColumn) {}
+  IncludeGuard(Style.IndentPPDirectives == FormatStyle::PPDIS_None
+   ? IG_Rejected
+   : IG_Inited),
+  IncludeGuardToken(nullptr), FirstStartColumn(FirstStartColumn) {}
 
 void UnwrappedLineParser::reset() {
   PPBranchLevel = -1;
-  IfNdefCondition = nullptr;
-  FoundIncludeGuardStart = false;
-  IncludeGuardRejected = false;
+  IncludeGuard = IG_Inited;
+  IncludeGuardToken = nullptr;
   Line.reset(new UnwrappedLine);
   CommentsBeforeNextToken.clear();
   FormatTok = nullptr;
@@ -264,6 +265,14 @@
 
 readToken();
 parseFile();
+
+// If we found an include guard then all preprocessor directives (other than
+// the guard) are over-indented by one.
+if (IncludeGuard == IG_Found)
+  for (auto  : Lines)
+if (Line.InPPDirective && Line.Level > 0)
+  --Line.Level;
+
 // Create line with eof token.
 pushToken(FormatTok);
 addUnwrappedLine();
@@ -712,26 +721,28 @@
   // 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) {
+  if (IncludeGuard == IG_Inited && MaybeIncludeGuard) {
 for (auto  : Lines) {
   if (!Line.Tokens.front().Tok->is(tok::comment)) {
 MaybeIncludeGuard = false;
-IncludeGuardRejected = true;
+IncludeGuard = IG_Rejected;
 break;
   }
 }
   }
   --PPBranchLevel;
   parsePPUnknown();
   

[PATCH] D42034: [clang-format] In tests, expected code should be format-stable

[Mark Zeren via Phabricator via cfe-commits]

mzeren-vmw created this revision.
mzeren-vmw added reviewers: klimek, krasimir, euhlmann.
Herald added a subscriber: cfe-commits.

Extend the verifyFormat helper function to check that the expected text
is "stable". This provides some protection against bugs where formatting
results are ocilating between two forms, or continually change in some
other way.

Testing Done:

- Ran unit tests.

- Reproduced a known instability in preprocessor indentation which was caught 
by this new check (to be resolved in a later change.)


Repository:
  rC Clang

https://reviews.llvm.org/D42034

Files:
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -72,6 +72,7 @@
 
   void verifyFormat(llvm::StringRef Expected, llvm::StringRef Code,
 const FormatStyle  = getLLVMStyle()) {
+EXPECT_EQ(Expected.str(), format(Expected, Style));
 EXPECT_EQ(Expected.str(), format(Code, Style));
 if (Style.Language == FormatStyle::LK_Cpp) {
   // Objective-C++ is a superset of C++, so everything checked for C++


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -72,6 +72,7 @@
 
   void verifyFormat(llvm::StringRef Expected, llvm::StringRef Code,
 const FormatStyle  = getLLVMStyle()) {
+EXPECT_EQ(Expected.str(), format(Expected, Style));
 EXPECT_EQ(Expected.str(), format(Code, Style));
 if (Style.Language == FormatStyle::LK_Cpp) {
   // Objective-C++ is a superset of C++, so everything checked for C++
___
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

[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