[llvm-branch-commits] [clang] c7d7ace - [clang-format] Rework Whitesmiths mode to use line-level values in UnwrappedLineParser
via llvm-branch-commits
llvm-branch-commits at lists.llvm.org
Tue Jun 15 11:28:29 PDT 2021
Author: Tim Wojtulewicz
Date: 2021-06-15T19:22:44+01:00
New Revision: c7d7ace46258b04aa4b5df08952bfebc6fc4ce94
URL: https://github.com/llvm/llvm-project/commit/c7d7ace46258b04aa4b5df08952bfebc6fc4ce94
DIFF: https://github.com/llvm/llvm-project/commit/c7d7ace46258b04aa4b5df08952bfebc6fc4ce94.diff
LOG: [clang-format] Rework Whitesmiths mode to use line-level values in UnwrappedLineParser
This commit removes the old way of handling Whitesmiths mode in favor of just setting the
levels during parsing and letting the formatter handle it from there. It requires a bit of
special-casing during the parsing, but ends up a bit cleaner than before. It also removes
some of switch/case unit tests that don't really make much sense when dealing with
Whitesmiths.
Differential Revision: https://reviews.llvm.org/D94500
(cherry picked from commit f7f9f94b2e2b4c714bac9036f6b73a3df42daaff)
Added:
Modified:
clang/docs/ReleaseNotes.rst
clang/lib/Format/UnwrappedLineFormatter.cpp
clang/lib/Format/UnwrappedLineParser.cpp
clang/lib/Format/UnwrappedLineParser.h
clang/unittests/Format/FormatTest.cpp
Removed:
################################################################################
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 4cc1b0b9d2cf3..460a62734e906 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -389,6 +389,9 @@ clang-format
``AlignConsecutiveDeclarations`` and ``AlignConsecutiveMacros`` have been modified to allow
alignment across empty lines and/or comments.
+- Support for Whitesmiths has been improved, with fixes for ``namespace`` blocks
+ and ``case`` blocks and labels.
+
libclang
--------
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 5dd0ccdfa6fd1..7d197310e65bb 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -1281,13 +1281,6 @@ void UnwrappedLineFormatter::formatFirstToken(
if (Newlines)
Indent = NewlineIndent;
- // If in Whitemsmiths mode, indent start and end of blocks
- if (Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths) {
- if (RootToken.isOneOf(tok::l_brace, tok::r_brace, tok::kw_case,
- tok::kw_default))
- Indent += Style.IndentWidth;
- }
-
// Preprocessor directives get indented before the hash only if specified
if (Style.IndentPPDirectives != FormatStyle::PPDIS_BeforeHash &&
(Line.Type == LT_PreprocessorDirective ||
diff --git a/clang/lib/Format/UnwrappedLineParser.cpp b/clang/lib/Format/UnwrappedLineParser.cpp
index f689a6361a3ad..bec18bd5d8df4 100644
--- a/clang/lib/Format/UnwrappedLineParser.cpp
+++ b/clang/lib/Format/UnwrappedLineParser.cpp
@@ -579,17 +579,23 @@ size_t UnwrappedLineParser::computePPHash() const {
return h;
}
-void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, bool AddLevel,
- bool MunchSemi) {
+void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, unsigned AddLevels,
+ bool MunchSemi,
+ bool UnindentWhitesmithsBraces) {
assert(FormatTok->isOneOf(tok::l_brace, TT_MacroBlockBegin) &&
"'{' or macro block token expected");
const bool MacroBlock = FormatTok->is(TT_MacroBlockBegin);
FormatTok->setBlockKind(BK_Block);
+ // For Whitesmiths mode, jump to the next level prior to skipping over the
+ // braces.
+ if (AddLevels > 0 && Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths)
+ ++Line->Level;
+
size_t PPStartHash = computePPHash();
unsigned InitialLevel = Line->Level;
- nextToken(/*LevelDifference=*/AddLevel ? 1 : 0);
+ nextToken(/*LevelDifference=*/AddLevels);
if (MacroBlock && FormatTok->is(tok::l_paren))
parseParens();
@@ -602,10 +608,16 @@ void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, bool AddLevel,
? (UnwrappedLine::kInvalidIndex)
: (CurrentLines->size() - 1 - NbPreprocessorDirectives);
+ // Whitesmiths is weird here. The brace needs to be indented for the namespace
+ // block, but the block itself may not be indented depending on the style
+ // settings. This allows the format to back up one level in those cases.
+ if (UnindentWhitesmithsBraces)
+ --Line->Level;
+
ScopedDeclarationState DeclarationState(*Line, DeclarationScopeStack,
MustBeDeclaration);
- if (AddLevel)
- ++Line->Level;
+ if (AddLevels > 0u && Style.BreakBeforeBraces != FormatStyle::BS_Whitesmiths)
+ Line->Level += AddLevels;
parseLevel(/*HasOpeningBrace=*/true);
if (eof())
@@ -621,7 +633,7 @@ void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, bool AddLevel,
size_t PPEndHash = computePPHash();
// Munch the closing brace.
- nextToken(/*LevelDifference=*/AddLevel ? -1 : 0);
+ nextToken(/*LevelDifference=*/-AddLevels);
if (MacroBlock && FormatTok->is(tok::l_paren))
parseParens();
@@ -637,6 +649,7 @@ void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, bool AddLevel,
nextToken();
Line->Level = InitialLevel;
+ FormatTok->setBlockKind(BK_Block);
if (PPStartHash == PPEndHash) {
Line->MatchingOpeningBlockLineIndex = OpeningLineIndex;
@@ -2128,15 +2141,34 @@ void UnwrappedLineParser::parseNamespace() {
if (ShouldBreakBeforeBrace(Style, InitialToken))
addUnwrappedLine();
- bool AddLevel = Style.NamespaceIndentation == FormatStyle::NI_All ||
- (Style.NamespaceIndentation == FormatStyle::NI_Inner &&
- DeclarationScopeStack.size() > 1);
- parseBlock(/*MustBeDeclaration=*/true, AddLevel);
+ unsigned AddLevels =
+ Style.NamespaceIndentation == FormatStyle::NI_All ||
+ (Style.NamespaceIndentation == FormatStyle::NI_Inner &&
+ DeclarationScopeStack.size() > 1)
+ ? 1u
+ : 0u;
+ bool ManageWhitesmithsBraces =
+ AddLevels == 0u &&
+ Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths;
+
+ // If we're in Whitesmiths mode, indent the brace if we're not indenting
+ // the whole block.
+ if (ManageWhitesmithsBraces)
+ ++Line->Level;
+
+ parseBlock(/*MustBeDeclaration=*/true, AddLevels,
+ /*MunchSemi=*/true,
+ /*UnindentWhitesmithsBraces=*/ManageWhitesmithsBraces);
+
// Munch the semicolon after a namespace. This is more common than one would
// think. Putting the semicolon into its own line is very ugly.
if (FormatTok->Tok.is(tok::semi))
nextToken();
- addUnwrappedLine();
+
+ addUnwrappedLine(AddLevels > 0 ? LineLevel::Remove : LineLevel::Keep);
+
+ if (ManageWhitesmithsBraces)
+ --Line->Level;
}
// FIXME: Add error handling.
}
@@ -2222,6 +2254,11 @@ void UnwrappedLineParser::parseDoWhile() {
return;
}
+ // If in Whitesmiths mode, the line with the while() needs to be indented
+ // to the same level as the block.
+ if (Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths)
+ ++Line->Level;
+
nextToken();
parseStructuralElement();
}
@@ -2234,25 +2271,19 @@ void UnwrappedLineParser::parseLabel(bool LeftAlignLabel) {
if (LeftAlignLabel)
Line->Level = 0;
- bool RemoveWhitesmithsCaseIndent =
- (!Style.IndentCaseBlocks &&
- Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths);
-
- if (RemoveWhitesmithsCaseIndent)
- --Line->Level;
-
if (!Style.IndentCaseBlocks && CommentsBeforeNextToken.empty() &&
FormatTok->Tok.is(tok::l_brace)) {
- CompoundStatementIndenter Indenter(
- this, Line->Level, Style.BraceWrapping.AfterCaseLabel,
- Style.BraceWrapping.IndentBraces || RemoveWhitesmithsCaseIndent);
+ CompoundStatementIndenter Indenter(this, Line->Level,
+ Style.BraceWrapping.AfterCaseLabel,
+ Style.BraceWrapping.IndentBraces);
parseBlock(/*MustBeDeclaration=*/false);
if (FormatTok->Tok.is(tok::kw_break)) {
if (Style.BraceWrapping.AfterControlStatement ==
FormatStyle::BWACS_Always) {
addUnwrappedLine();
- if (RemoveWhitesmithsCaseIndent) {
+ if (!Style.IndentCaseBlocks &&
+ Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths) {
Line->Level++;
}
}
@@ -2920,17 +2951,29 @@ LLVM_ATTRIBUTE_UNUSED static void printDebugInfo(const UnwrappedLine &Line,
llvm::dbgs() << "\n";
}
-void UnwrappedLineParser::addUnwrappedLine() {
+void UnwrappedLineParser::addUnwrappedLine(LineLevel AdjustLevel) {
if (Line->Tokens.empty())
return;
LLVM_DEBUG({
if (CurrentLines == &Lines)
printDebugInfo(*Line);
});
+
+ // If this line closes a block when in Whitesmiths mode, remember that
+ // information so that the level can be decreased after the line is added.
+ // This has to happen after the addition of the line since the line itself
+ // needs to be indented.
+ bool ClosesWhitesmithsBlock =
+ Line->MatchingOpeningBlockLineIndex != UnwrappedLine::kInvalidIndex &&
+ Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths;
+
CurrentLines->push_back(std::move(*Line));
Line->Tokens.clear();
Line->MatchingOpeningBlockLineIndex = UnwrappedLine::kInvalidIndex;
Line->FirstStartColumn = 0;
+
+ if (ClosesWhitesmithsBlock && AdjustLevel == LineLevel::Remove)
+ --Line->Level;
if (CurrentLines == &Lines && !PreprocessorDirectives.empty()) {
CurrentLines->append(
std::make_move_iterator(PreprocessorDirectives.begin()),
diff --git a/clang/lib/Format/UnwrappedLineParser.h b/clang/lib/Format/UnwrappedLineParser.h
index 02b328cb72de6..ce135fac5e578 100644
--- a/clang/lib/Format/UnwrappedLineParser.h
+++ b/clang/lib/Format/UnwrappedLineParser.h
@@ -85,8 +85,9 @@ class UnwrappedLineParser {
void reset();
void parseFile();
void parseLevel(bool HasOpeningBrace);
- void parseBlock(bool MustBeDeclaration, bool AddLevel = true,
- bool MunchSemi = true);
+ void parseBlock(bool MustBeDeclaration, unsigned AddLevels = 1u,
+ bool MunchSemi = true,
+ bool UnindentWhitesmithsBraces = false);
void parseChildBlock();
void parsePPDirective();
void parsePPDefine();
@@ -140,7 +141,12 @@ class UnwrappedLineParser {
bool tryToParsePropertyAccessor();
void tryToParseJSFunction();
bool tryToParseSimpleAttribute();
- void addUnwrappedLine();
+
+ // Used by addUnwrappedLine to denote whether to keep or remove a level
+ // when resetting the line state.
+ enum class LineLevel { Remove, Keep };
+
+ void addUnwrappedLine(LineLevel AdjustLevel = LineLevel::Remove);
bool eof() const;
// LevelDifference is the
diff erence of levels after and before the current
// token. For example:
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index ed26bb8a7150e..45278fdb69e65 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -14741,6 +14741,7 @@ TEST_F(FormatTest, WhitesmithsBraceBreaking) {
WhitesmithsBraceStyle);
*/
+ WhitesmithsBraceStyle.NamespaceIndentation = FormatStyle::NI_None;
verifyFormat("namespace a\n"
" {\n"
"class A\n"
@@ -14765,6 +14766,89 @@ TEST_F(FormatTest, WhitesmithsBraceBreaking) {
" } // namespace a",
WhitesmithsBraceStyle);
+ verifyFormat("namespace a\n"
+ " {\n"
+ "namespace b\n"
+ " {\n"
+ "class A\n"
+ " {\n"
+ " void f()\n"
+ " {\n"
+ " if (true)\n"
+ " {\n"
+ " a();\n"
+ " b();\n"
+ " }\n"
+ " }\n"
+ " void g()\n"
+ " {\n"
+ " return;\n"
+ " }\n"
+ " };\n"
+ "struct B\n"
+ " {\n"
+ " int x;\n"
+ " };\n"
+ " } // namespace b\n"
+ " } // namespace a",
+ WhitesmithsBraceStyle);
+
+ WhitesmithsBraceStyle.NamespaceIndentation = FormatStyle::NI_Inner;
+ verifyFormat("namespace a\n"
+ " {\n"
+ "namespace b\n"
+ " {\n"
+ " class A\n"
+ " {\n"
+ " void f()\n"
+ " {\n"
+ " if (true)\n"
+ " {\n"
+ " a();\n"
+ " b();\n"
+ " }\n"
+ " }\n"
+ " void g()\n"
+ " {\n"
+ " return;\n"
+ " }\n"
+ " };\n"
+ " struct B\n"
+ " {\n"
+ " int x;\n"
+ " };\n"
+ " } // namespace b\n"
+ " } // namespace a",
+ WhitesmithsBraceStyle);
+
+ WhitesmithsBraceStyle.NamespaceIndentation = FormatStyle::NI_All;
+ verifyFormat("namespace a\n"
+ " {\n"
+ " namespace b\n"
+ " {\n"
+ " class A\n"
+ " {\n"
+ " void f()\n"
+ " {\n"
+ " if (true)\n"
+ " {\n"
+ " a();\n"
+ " b();\n"
+ " }\n"
+ " }\n"
+ " void g()\n"
+ " {\n"
+ " return;\n"
+ " }\n"
+ " };\n"
+ " struct B\n"
+ " {\n"
+ " int x;\n"
+ " };\n"
+ " } // namespace b\n"
+ " } // namespace a",
+ WhitesmithsBraceStyle);
+
verifyFormat("void f()\n"
" {\n"
" if (true)\n"
@@ -14799,7 +14883,7 @@ TEST_F(FormatTest, WhitesmithsBraceBreaking) {
" }\n",
WhitesmithsBraceStyle);
- WhitesmithsBraceStyle.IndentCaseBlocks = true;
+ WhitesmithsBraceStyle.IndentCaseLabels = true;
verifyFormat("void switchTest1(int a)\n"
" {\n"
" switch (a)\n"
@@ -14807,7 +14891,7 @@ TEST_F(FormatTest, WhitesmithsBraceBreaking) {
" case 2:\n"
" {\n"
" }\n"
- " break;\n"
+ " break;\n"
" }\n"
" }\n",
WhitesmithsBraceStyle);
@@ -14817,7 +14901,7 @@ TEST_F(FormatTest, WhitesmithsBraceBreaking) {
" switch (a)\n"
" {\n"
" case 0:\n"
- " break;\n"
+ " break;\n"
" case 1:\n"
" {\n"
" break;\n"
@@ -14825,9 +14909,9 @@ TEST_F(FormatTest, WhitesmithsBraceBreaking) {
" case 2:\n"
" {\n"
" }\n"
- " break;\n"
+ " break;\n"
" default:\n"
- " break;\n"
+ " break;\n"
" }\n"
" }\n",
WhitesmithsBraceStyle);
@@ -14840,17 +14924,17 @@ TEST_F(FormatTest, WhitesmithsBraceBreaking) {
" {\n"
" foo(x);\n"
" }\n"
- " break;\n"
+ " break;\n"
" default:\n"
" {\n"
" foo(1);\n"
" }\n"
- " break;\n"
+ " break;\n"
" }\n"
" }\n",
WhitesmithsBraceStyle);
- WhitesmithsBraceStyle.IndentCaseBlocks = false;
+ WhitesmithsBraceStyle.IndentCaseLabels = false;
verifyFormat("void switchTest4(int a)\n"
" {\n"
More information about the llvm-branch-commits
mailing list