Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions clang/docs/ClangFormatStyleOptions.rst
Original file line number Diff line number Diff line change
Expand Up @@ -3828,6 +3828,21 @@ the configuration (without a prefix: ``Auto``).
This is an experimental flag, that might go away or be renamed. Do
not use this in config files, etc. Use at your own risk.

.. _ExportBlockIndentation:

**ExportBlockIndentation** (``Boolean``) :versionbadge:`clang-format 20` :ref:`¶ <ExportBlockIndentation>`
If ``true``, clang-format will indent the body of an ``export { ... }``
block. This doesn't affect the formatting of anything else related to
exported declarations.

.. code-block:: c++

true: false:
export { vs. export {
void foo(); void foo();
void bar(); void bar();
} }

.. _FixNamespaceComments:

**FixNamespaceComments** (``Boolean``) :versionbadge:`clang-format 5` :ref:`¶ <FixNamespaceComments>`
Expand Down
1 change: 1 addition & 0 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -554,6 +554,7 @@ clang-format
------------

- Adds ``BreakBinaryOperations`` option.
- Adds the ``ExportBlockIndentation`` option.

libclang
--------
Expand Down
14 changes: 14 additions & 0 deletions clang/include/clang/Format/Format.h
Original file line number Diff line number Diff line change
Expand Up @@ -2655,6 +2655,19 @@ struct FormatStyle {
/// \version 3.7
bool ExperimentalAutoDetectBinPacking;

/// If ``true``, clang-format will indent the body of an ``export { ... }``
/// block. This doesn't affect the formatting of anything else related to
/// exported declarations.
/// \code
/// true: false:
/// export { vs. export {
/// void foo(); void foo();
/// void bar(); void bar();
/// } }
/// \endcode
/// \version 20
bool ExportBlockIndentation;

/// If ``true``, clang-format adds missing namespace end comments for
/// namespaces and fixes invalid existing ones. This doesn't affect short
/// namespaces, which are controlled by ``ShortNamespaceLines``.
Expand Down Expand Up @@ -5131,6 +5144,7 @@ struct FormatStyle {
EmptyLineBeforeAccessModifier == R.EmptyLineBeforeAccessModifier &&
ExperimentalAutoDetectBinPacking ==
R.ExperimentalAutoDetectBinPacking &&
ExportBlockIndentation == R.ExportBlockIndentation &&
FixNamespaceComments == R.FixNamespaceComments &&
ForEachMacros == R.ForEachMacros &&
IncludeStyle.IncludeBlocks == R.IncludeStyle.IncludeBlocks &&
Expand Down
2 changes: 2 additions & 0 deletions clang/lib/Format/Format.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1016,6 +1016,7 @@ template <> struct MappingTraits<FormatStyle> {
Style.EmptyLineBeforeAccessModifier);
IO.mapOptional("ExperimentalAutoDetectBinPacking",
Style.ExperimentalAutoDetectBinPacking);
IO.mapOptional("ExportBlockIndentation", Style.ExportBlockIndentation);
IO.mapOptional("FixNamespaceComments", Style.FixNamespaceComments);
IO.mapOptional("ForEachMacros", Style.ForEachMacros);
IO.mapOptional("IfMacros", Style.IfMacros);
Expand Down Expand Up @@ -1522,6 +1523,7 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
LLVMStyle.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Never;
LLVMStyle.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_LogicalBlock;
LLVMStyle.ExperimentalAutoDetectBinPacking = false;
LLVMStyle.ExportBlockIndentation = false;
LLVMStyle.FixNamespaceComments = true;
LLVMStyle.ForEachMacros.push_back("foreach");
LLVMStyle.ForEachMacros.push_back("Q_FOREACH");
Expand Down
48 changes: 29 additions & 19 deletions clang/lib/Format/UnwrappedLineParser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1616,6 +1616,10 @@ void UnwrappedLineParser::parseStructuralElement(
parseNamespace();
return;
}
if (FormatTok->is(tok::l_brace)) {
parseCXXExportBlock();
return;
}
if (FormatTok->is(Keywords.kw_import) && parseModuleImport())
return;
}
Expand Down Expand Up @@ -3075,6 +3079,26 @@ void UnwrappedLineParser::parseTryCatch() {
addUnwrappedLine();
}

void UnwrappedLineParser::parseNamespaceOrExportBlock(unsigned 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;

// Munch the semicolon after a namespace. This is more common than one would
// think. Putting the semicolon into its own line is very ugly.
parseBlock(/*MustBeDeclaration=*/true, AddLevels, /*MunchSemi=*/true,
/*KeepBraces=*/true, /*IfKind=*/nullptr, ManageWhitesmithsBraces);

addUnwrappedLine(AddLevels > 0 ? LineLevel::Remove : LineLevel::Keep);

if (ManageWhitesmithsBraces)
--Line->Level;
}

void UnwrappedLineParser::parseNamespace() {
assert(FormatTok->isOneOf(tok::kw_namespace, TT_NamespaceMacro) &&
"'namespace' expected");
Expand Down Expand Up @@ -3107,29 +3131,15 @@ void UnwrappedLineParser::parseNamespace() {
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;

// Munch the semicolon after a namespace. This is more common than one would
// think. Putting the semicolon into its own line is very ugly.
parseBlock(/*MustBeDeclaration=*/true, AddLevels, /*MunchSemi=*/true,
/*KeepBraces=*/true, /*IfKind=*/nullptr,
ManageWhitesmithsBraces);

addUnwrappedLine(AddLevels > 0 ? LineLevel::Remove : LineLevel::Keep);

if (ManageWhitesmithsBraces)
--Line->Level;
parseNamespaceOrExportBlock(AddLevels);
}
// FIXME: Add error handling.
}

void UnwrappedLineParser::parseCXXExportBlock() {
parseNamespaceOrExportBlock(Style.ExportBlockIndentation ? 1 : 0);
}

void UnwrappedLineParser::parseNew() {
assert(FormatTok->is(tok::kw_new) && "'new' expected");
nextToken();
Expand Down
2 changes: 2 additions & 0 deletions clang/lib/Format/UnwrappedLineParser.h
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,8 @@ class UnwrappedLineParser {
void parseRequiresClause(FormatToken *RequiresToken);
void parseRequiresExpression(FormatToken *RequiresToken);
void parseConstraintExpression();
void parseCXXExportBlock();
void parseNamespaceOrExportBlock(unsigned AddLevels);
void parseJavaEnumBody();
// Parses a record (aka class) as a top level element. If ParseAsExpr is true,
// parses the record as a child block, i.e. if the class declaration is an
Expand Down
30 changes: 26 additions & 4 deletions clang/unittests/Format/FormatTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9040,6 +9040,31 @@ TEST_F(FormatTest, AdaptiveOnePerLineFormatting) {
Style);
}

TEST_F(FormatTest, ExportBlockIndentation) {
FormatStyle Style = getLLVMStyleWithColumns(80);
Style.ExportBlockIndentation = true;
verifyFormat("export {\n"
" int x;\n"
" int y;\n"
"}",
"export {\n"
"int x;\n"
"int y;\n"
"}",
Style);

Style.ExportBlockIndentation = false;
verifyFormat("export {\n"
"int x;\n"
"int y;\n"
"}",
"export {\n"
" int x;\n"
" int y;\n"
"}",
Style);
}

TEST_F(FormatTest, FormatsBuilderPattern) {
verifyFormat("return llvm::StringSwitch<Reference::Kind>(name)\n"
" .StartsWith(\".eh_frame_hdr\", ORDER_EH_FRAMEHDR)\n"
Expand Down Expand Up @@ -26588,10 +26613,7 @@ TEST_F(FormatTest, Cpp20ModulesSupport) {
" int foo;\n"
"};",
Style);
verifyFormat("export {\n"
" int foo;\n"
"};",
Style);
verifyFormat("export { int foo; };", Style);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's going on here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't fix the tests by changing them... here you make the decision that people want a collapsed export {} what makes you say thats what everyone wants?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, makes sense. As far as I understand it, were were previously just parsing this as a compound statement, which afaik isn’t formatted on a single line by default, but maybe namespaces are, but I’m candidly not quite sure what’s causing this to be formatted on one line... I think this has something to do with the fact that I used parseBlock for this, but I’ll have to look into it a bit more.

Do we want a separate option for this (e.g. something like AllowShortExportBlocksOnASingleLine) or should that just fall under AllowShortBlocksOnASingleLine?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be an option to en/disable it, if it can be shared with other things, or be exclusively for export is to be discussed. I have no strong opinion, but would lean to an extra option, because someone will ask for it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense; I’ll hopefully have time to look into this next week because I’ve been a bit busy recently.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now controlled by AllowShortBlocksOnASingleLine. I opted against adding a separate option for export blocks for now because realistically, e.g. export { int x; } is not going to be very common considering that you can just write export int x;.

verifyFormat("export export char const *hello() { return \"hello\"; }");

verifyFormat("import bar;", Style);
Expand Down
Loading