Skip to content

Commit

Permalink
[clang-format] Rework Whitesmiths mode to use line-level values in Un…
Browse files Browse the repository at this point in the history 
…wrappedLineParser

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
  • Loading branch information
@timwoj @HazardyKnusperkeks
timwoj authored and HazardyKnusperkeks committed Mar 5, 2021
1 parent 7b02794 commit f7f9f94
Show file tree
Hide file tree
Showing 5 changed files with 160 additions and 33 deletions.
3 changes: 3 additions & 0 deletions clang/docs/ReleaseNotes.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,9 @@ clang-format
- Option ``ShortNamespaceLines`` has been added to give better control
over ``FixNamespaceComments`` when determining a namespace length.

- Support for Whitesmiths has been improved, with fixes for ``namespace`` blocks
and ``case`` blocks and labels.

libclang
--------

Expand Down
7 changes: 0 additions & 7 deletions clang/lib/Format/UnwrappedLineFormatter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1286,13 +1286,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 ||
Expand Down
73 changes: 57 additions & 16 deletions clang/lib/Format/UnwrappedLineParser.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -580,12 +580,18 @@ size_t UnwrappedLineParser::computePPHash() const {
}

void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, unsigned AddLevels,
bool MunchSemi) {
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;
Expand All @@ -602,9 +608,16 @@ void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, unsigned AddLevels,
? (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);
Line->Level += AddLevels;
if (AddLevels > 0u && Style.BreakBeforeBraces != FormatStyle::BS_Whitesmiths)
Line->Level += AddLevels;
parseLevel(/*HasOpeningBrace=*/true);

if (eof())
Expand Down Expand Up @@ -636,6 +649,7 @@ void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, unsigned AddLevels,
nextToken();

Line->Level = InitialLevel;
FormatTok->setBlockKind(BK_Block);

if (PPStartHash == PPEndHash) {
Line->MatchingOpeningBlockLineIndex = OpeningLineIndex;
Expand Down Expand Up @@ -2133,12 +2147,28 @@ void UnwrappedLineParser::parseNamespace() {
DeclarationScopeStack.size() > 1)
? 1u
: 0u;
parseBlock(/*MustBeDeclaration=*/true, AddLevels);
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.
}
Expand Down Expand Up @@ -2224,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();
}
Expand All @@ -2236,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++;
}
}
Expand Down Expand Up @@ -2922,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()),
Expand Down
10 changes: 8 additions & 2 deletions clang/lib/Format/UnwrappedLineParser.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,8 @@ class UnwrappedLineParser {
void parseFile();
void parseLevel(bool HasOpeningBrace);
void parseBlock(bool MustBeDeclaration, unsigned AddLevels = 1u,
bool MunchSemi = true);
bool MunchSemi = true,
bool UnindentWhitesmithsBraces = false);
void parseChildBlock();
void parsePPDirective();
void parsePPDefine();
Expand Down Expand Up @@ -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 difference of levels after and before the current
// token. For example:
Expand Down
100 changes: 92 additions & 8 deletions clang/unittests/Format/FormatTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14767,6 +14767,7 @@ TEST_F(FormatTest, WhitesmithsBraceBreaking) {
WhitesmithsBraceStyle);
*/

WhitesmithsBraceStyle.NamespaceIndentation = FormatStyle::NI_None;
verifyFormat("namespace a\n"
" {\n"
"class A\n"
Expand All @@ -14791,6 +14792,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"
Expand Down Expand Up @@ -14825,15 +14909,15 @@ TEST_F(FormatTest, WhitesmithsBraceBreaking) {
" }\n",
WhitesmithsBraceStyle);

WhitesmithsBraceStyle.IndentCaseBlocks = true;
WhitesmithsBraceStyle.IndentCaseLabels = true;
verifyFormat("void switchTest1(int a)\n"
" {\n"
" switch (a)\n"
" {\n"
" case 2:\n"
" {\n"
" }\n"
" break;\n"
" break;\n"
" }\n"
" }\n",
WhitesmithsBraceStyle);
Expand All @@ -14843,17 +14927,17 @@ TEST_F(FormatTest, WhitesmithsBraceBreaking) {
" switch (a)\n"
" {\n"
" case 0:\n"
" break;\n"
" break;\n"
" case 1:\n"
" {\n"
" break;\n"
" }\n"
" case 2:\n"
" {\n"
" }\n"
" break;\n"
" break;\n"
" default:\n"
" break;\n"
" break;\n"
" }\n"
" }\n",
WhitesmithsBraceStyle);
Expand All @@ -14866,17 +14950,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"
Expand Down

0 comments on commit f7f9f94

Please sign in to comment.