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 cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits