MyDeveloperDay created this revision.
MyDeveloperDay added reviewers: HazardyKnusperkeks, owenpan, curdeius, rymiel,
Eugene.Zelenko.
MyDeveloperDay added projects: clang, clang-format.
Herald added a project: All.
MyDeveloperDay requested review of this revision.
Fixes: #58217
This change is to remove extraneous and unnecessary ';' from after a function
declaration, its off by default and carries the same "code modification"
warning as some of our other code manipulating changes.
int max(int a, int b)
{
return a>b?a:b;
};
class Foo
{
int getSomething() const { return something; };
};
At first, I thought this was a minor problem and not worth anything other than
using `-Wextra-semi/-Wextra-semi-stmt` as pointed out in the issue comments.
But clang-format is used by people who may not use the clang compiler, and not
all compilers have this extra semi warning (AFAIK)
However, I ran this over the clang-codebase and realized (in clang and even the
clang-format Tests!) how prevalent this is.
This is implemented very much on the same lines as @owenpan does for removing
the `{}` with RemoveBracesLLVM, there is some repeated code that we could
rationalize down if accepted. (I didn't want to be too invasive on that work)
This is definitely one of those changes that it would be nice for those of us
that use "clang-format on save" could get without having to go through a
compile phase in order to catch the warnings.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D135466
Files:
clang/docs/ClangFormatStyleOptions.rst
clang/docs/ReleaseNotes.rst
clang/include/clang/Format/Format.h
clang/lib/Format/Format.cpp
clang/lib/Format/UnwrappedLineParser.cpp
clang/lib/Format/UnwrappedLineParser.h
clang/unittests/Format/FormatTest.cpp
Index: clang/unittests/Format/FormatTest.cpp
===================================================================
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -20281,6 +20281,7 @@
CHECK_PARSE_BOOL(Cpp11BracedListStyle);
CHECK_PARSE_BOOL(ReflowComments);
CHECK_PARSE_BOOL(RemoveBracesLLVM);
+ CHECK_PARSE_BOOL(RemoveSemiColons);
CHECK_PARSE_BOOL(SortUsingDeclarations);
CHECK_PARSE_BOOL(SpacesInParentheses);
CHECK_PARSE_BOOL(SpacesInSquareBrackets);
@@ -26740,6 +26741,22 @@
"inline bool var = is_integral_v<T> && is_signed_v<T>;");
}
+TEST_F(FormatTest, RemoveSemiColons) {
+ FormatStyle Style = getLLVMStyle();
+ Style.InsertBraces = true;
+
+ verifyFormat("int max(int a, int b) { return a > b ? a : b; }",
+ "int max(int a, int b) { return a > b ? a : b; };", Style);
+
+ verifyFormat("class Foo {\n"
+ " int getSomething() const { return something; }\n"
+ "};",
+ "class Foo {\n"
+ " int getSomething() const { return something; };\n"
+ "};",
+ Style);
+}
+
} // namespace
} // namespace format
} // namespace clang
Index: clang/lib/Format/UnwrappedLineParser.h
===================================================================
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -111,7 +111,8 @@
bool KeepBraces = true, IfStmtKind *IfKind = nullptr,
bool UnindentWhitesmithsBraces = false,
bool CanContainBracedList = true,
- TokenType NextLBracesType = TT_Unknown);
+ TokenType NextLBracesType = TT_Unknown,
+ bool IsFunctionBlock = false);
void parseChildBlock(bool CanContainBracedList = true,
TokenType NextLBracesType = TT_Unknown);
void parsePPDirective();
Index: clang/lib/Format/UnwrappedLineParser.cpp
===================================================================
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -839,7 +839,8 @@
FormatToken *UnwrappedLineParser::parseBlock(
bool MustBeDeclaration, unsigned AddLevels, bool MunchSemi, bool KeepBraces,
IfStmtKind *IfKind, bool UnindentWhitesmithsBraces,
- bool CanContainBracedList, TokenType NextLBracesType) {
+ bool CanContainBracedList, TokenType NextLBracesType,
+ bool IsFunctionBlock) {
auto HandleVerilogBlockLabel = [this]() {
// ":" name
if (Style.isVerilog() && FormatTok->is(tok::colon)) {
@@ -978,8 +979,17 @@
parseStructuralElement();
}
- if (MunchSemi && FormatTok->is(tok::semi))
+ // When this is a function block and there is an unnecessary semicolon
+ // afterwards then mark it as optional (so the RemoveSemi pass can get rid of
+ // it later).
+ if (MunchSemi && FormatTok->is(tok::semi)) {
+ if (IsFunctionBlock) {
+ FormatToken *Previous = Tokens->getPreviousToken();
+ if (Previous && Previous->is(tok::r_brace))
+ FormatTok->Optional = true;
+ }
nextToken();
+ }
if (PPStartHash == PPEndHash) {
Line->MatchingOpeningBlockLineIndex = OpeningLineIndex;
@@ -1897,7 +1907,11 @@
addUnwrappedLine();
}
FormatTok->setFinalizedType(TT_FunctionLBrace);
- parseBlock();
+ parseBlock(/*MustBeDeclaration=*/false, 1u, /*MunchSemi=*/true,
+ /*KeepBraces=*/true, nullptr,
+ /*UnindentWhitesmithsBraces=*/false,
+ /*CanContainBracedList=*/true, TT_Unknown,
+ /*IsFunctionBlock=*/true);
addUnwrappedLine();
return;
}
Index: clang/lib/Format/Format.cpp
===================================================================
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -852,6 +852,7 @@
IO.mapOptional("ReferenceAlignment", Style.ReferenceAlignment);
IO.mapOptional("ReflowComments", Style.ReflowComments);
IO.mapOptional("RemoveBracesLLVM", Style.RemoveBracesLLVM);
+ IO.mapOptional("RemoveSemiColons", Style.RemoveSemiColons);
IO.mapOptional("RequiresClausePosition", Style.RequiresClausePosition);
IO.mapOptional("SeparateDefinitionBlocks", Style.SeparateDefinitionBlocks);
IO.mapOptional("ShortNamespaceLines", Style.ShortNamespaceLines);
@@ -1297,6 +1298,7 @@
LLVMStyle.UseTab = FormatStyle::UT_Never;
LLVMStyle.ReflowComments = true;
LLVMStyle.RemoveBracesLLVM = false;
+ LLVMStyle.RemoveSemiColons = false;
LLVMStyle.SpacesInParentheses = false;
LLVMStyle.SpacesInSquareBrackets = false;
LLVMStyle.SpaceInEmptyBlock = false;
@@ -1915,7 +1917,59 @@
Token = Token->Next) {
if (!Token->Optional)
continue;
- assert(Token->isOneOf(tok::l_brace, tok::r_brace));
+ if (!Token->isOneOf(tok::l_brace, tok::r_brace))
+ continue;
+ auto Next = Token->Next;
+ assert(Next || Token == Line->Last);
+ if (!Next && NextLine)
+ Next = NextLine->First;
+ SourceLocation Start;
+ if (Next && Next->NewlinesBefore == 0 && Next->isNot(tok::eof)) {
+ Start = Token->Tok.getLocation();
+ Next->WhitespaceRange = Token->WhitespaceRange;
+ } else {
+ Start = Token->WhitespaceRange.getBegin();
+ }
+ const auto Range =
+ CharSourceRange::getCharRange(Start, Token->Tok.getEndLoc());
+ cantFail(Result.add(tooling::Replacement(SourceMgr, Range, "")));
+ }
+ }
+ }
+};
+
+class SemiRemover : public TokenAnalyzer {
+public:
+ SemiRemover(const Environment &Env, const FormatStyle &Style)
+ : TokenAnalyzer(Env, Style) {}
+
+ std::pair<tooling::Replacements, unsigned>
+ analyze(TokenAnnotator &Annotator,
+ SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
+ FormatTokenLexer &Tokens) override {
+ AffectedRangeMgr.computeAffectedLines(AnnotatedLines);
+ tooling::Replacements Result;
+ removeSemi(AnnotatedLines, Result);
+ return {Result, 0};
+ }
+
+private:
+ void removeSemi(SmallVectorImpl<AnnotatedLine *> &Lines,
+ tooling::Replacements &Result) {
+ const auto &SourceMgr = Env.getSourceManager();
+ const auto End = Lines.end();
+ for (auto I = Lines.begin(); I != End; ++I) {
+ const auto Line = *I;
+ removeSemi(Line->Children, Result);
+ if (!Line->Affected)
+ continue;
+ const auto NextLine = I + 1 == End ? nullptr : I[1];
+ for (auto Token = Line->First; Token && !Token->Finalized;
+ Token = Token->Next) {
+ if (!Token->Optional)
+ continue;
+ if (!Token->is(tok::semi))
+ continue;
auto Next = Token->Next;
assert(Next || Token == Line->Last);
if (!Next && NextLine)
@@ -3280,6 +3334,12 @@
});
}
+ if (Style.RemoveSemiColons) {
+ Passes.emplace_back([&](const Environment &Env) {
+ return SemiRemover(Env, Expanded).process(/*SkipAnnotation=*/true);
+ });
+ }
+
if (Style.FixNamespaceComments) {
Passes.emplace_back([&](const Environment &Env) {
return NamespaceEndCommentsFixer(Env, Expanded).process();
Index: clang/include/clang/Format/Format.h
===================================================================
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -3055,6 +3055,26 @@
/// \version 14
bool RemoveBracesLLVM;
+ /// Removes unnecessary semicolons from the function braces
+ /// \warning
+ /// Setting this option to `true` could lead to incorrect code formatting due
+ /// to clang-format's lack of complete semantic information. As such, extra
+ /// care should be taken to review code changes made by this option.
+ /// NOTE:
+ /// Setting this to false will not add `;` where they were missing
+ /// \endwarning
+ /// \code
+ /// false: true:
+ ///
+ /// int max(int a, int b) int max(int a, int b)
+ /// { {
+ /// return a>b?a:b; return a>b?a:b;
+ /// }; }
+ ///
+ /// \endcode
+ /// \version 16
+ bool RemoveSemiColons;
+
/// \brief The possible positions for the requires clause. The
/// ``IndentRequires`` option is only used if the ``requires`` is put on the
/// start of a line.
@@ -3969,6 +3989,7 @@
RawStringFormats == R.RawStringFormats &&
ReferenceAlignment == R.ReferenceAlignment &&
RemoveBracesLLVM == R.RemoveBracesLLVM &&
+ RemoveSemiColons == R.RemoveSemiColons &&
RequiresClausePosition == R.RequiresClausePosition &&
SeparateDefinitionBlocks == R.SeparateDefinitionBlocks &&
ShortNamespaceLines == R.ShortNamespaceLines &&
Index: clang/docs/ReleaseNotes.rst
===================================================================
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -529,6 +529,7 @@
clang-format
------------
+- Add `RemoveSemiColons` option for removing unnecessary `;` after a function definition.
clang-extdef-mapping
--------------------
Index: clang/docs/ClangFormatStyleOptions.rst
===================================================================
--- clang/docs/ClangFormatStyleOptions.rst
+++ clang/docs/ClangFormatStyleOptions.rst
@@ -3758,6 +3758,26 @@
}
}
+**RemoveSemiColons** (``Boolean``) :versionbadge:`clang-format 16`
+ Removes unnecessary semicolons from the function braces
+
+ .. warning::
+
+ Setting this option to `true` could lead to incorrect code formatting due
+ to clang-format's lack of complete semantic information. As such, extra
+ care should be taken to review code changes made by this option.
+ NOTE:
+ Setting this to false will not add `;` where they were missing
+
+ .. code-block:: c++
+
+ false: true:
+
+ int max(int a, int b) int max(int a, int b)
+ { {
+ return a>b?a:b; return a>b?a:b;
+ }; };
+
**RequiresClausePosition** (``RequiresClausePositionStyle``) :versionbadge:`clang-format 15`
The position of the ``requires`` clause.
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
