Page MenuHomePhabricator

[clang-format] Add support to remove unnecessary semicolons after function definition
ClosedPublic

Authored by MyDeveloperDay on Oct 7 2022, 10:27 AM.

Details

Summary

Fixes: https://github.com/llvm/llvm-project/issues/58217

This change is to remove extraneous and unnecessary ';' from after a function definition, 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.

Diff Detail

Event Timeline

MyDeveloperDay created this revision.Oct 7 2022, 10:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2022, 10:27 AM
MyDeveloperDay requested review of this revision.Oct 7 2022, 10:27 AM
lahwaacz added inline comments.Oct 7 2022, 11:56 AM
clang/docs/ClangFormatStyleOptions.rst
3769

Nit: the note should probably not be part of the warning.

3779

There is an extra ; in the true case ;-)

Maybe even extend the option to other unnecessary semicolons like int x = 5;; or other noop statements, one just has to be careful not to remove the semicolon when it's the sole if/loop body.

clang/include/clang/Format/Format.h
3076

The name is a bit hard, just from that it's not clear that this option is harmless (modulo bugs). Maybe something like the said warning RemoveExtraSemiColons?

clang/unittests/Format/FormatTest.cpp
26755

What happens with 2 (or more) semicolons?

@MyDeveloperDay Cool!

Maybe even extend the option to other unnecessary semicolons like int x = 5;; or other noop statements, one just has to be careful not to remove the semicolon when it's the sole if/loop body.

IMO we should keep it simple for now. We can always extend it to an enum or struct down the road, similar to what I plan to do with RemoveBracesLLVM.

clang/docs/ClangFormatStyleOptions.rst
3761–3762

Because "semicolon" is a single word. Also, FWIW I prefer the singular form here and would save the plural form for e.g. InsertBraces to mean a pair of braces.

3770

IMO we don't need the note as it goes without saying.

3779

If this file was generated from the Format.h header below, we have a bug in the dump_format_style.py script.

clang/include/clang/Format/Format.h
3069–3071

LLVM style.

3076

I'm ok with the short name, which makes the user to look it up in the documentation. The alternative would be something like RemoveSemicolonAfterFunctionBody.

clang/lib/Format/Format.cpp
1971

Nit.

clang/lib/Format/UnwrappedLineParser.cpp
843

We don't need to add the parameter. See below.

922–925

So that we don't need to add a parameter to parseBlock.

976

The while loop would address https://reviews.llvm.org/D135466#inline-1306331 but is optional IMO because it wouldn't distinguish };; and the following:

};
;
999–1003

This can be deleted. See above. Now we don't have to worry about if parseParens() and parseStructuralElement() would see a }; sequence.

lahwaacz added inline comments.Oct 7 2022, 11:22 PM
clang/lib/Format/UnwrappedLineParser.cpp
976

Does it matter? If you have struct foo {};; and format it with e.g. clang-format --style=LLVM, you'll get

struct foo {};
;

so IMO these _should_ be indistinguishible.

owenpan added inline comments.Oct 8 2022, 1:11 AM
clang/lib/Format/UnwrappedLineParser.cpp
976

It might matter when removing the semicolons. We can keep the loop if we have sufficient coverage in test cases. For example, we must ensure that

void foo() {}; //
; int bar;

is not formatted to

void foo() {} // int bar;

@MyDeveloperDay Cool!

Maybe even extend the option to other unnecessary semicolons like int x = 5;; or other noop statements, one just has to be careful not to remove the semicolon when it's the sole if/loop body.

IMO we should keep it simple for now. We can always extend it to an enum or struct down the road, similar to what I plan to do with RemoveBracesLLVM.

Thanks, everyone for all the reviews, I'll work on those.

I tend to agree here, I was really only focused on the extraneous ; after a function, I sort of feel catching a double coding ;; error or even adding a missing ; after a functions ) as prettier/vs code seems to do are different problem statements.

I certainly don't mind adding the capability to fix those too (in subsequent commits), I don't like to add too much in a single commit. (Especially the initial one that carries a lot of the infrastructure for a new feature).

MyDeveloperDay marked 16 inline comments as done.
  • Switch to the looping method and remove the need to change parseBlock (phew!) thanks @owenpan for the guidance
  • Add tests for double ;;
  • Change the name to RemoveSemicolon
MyDeveloperDay edited the summary of this revision. (Show Details)Oct 8 2022, 8:55 AM
MyDeveloperDay added inline comments.
clang/docs/ClangFormatStyleOptions.rst
3779

I wonder if I didn't do a final regeneration, lets check when I update

clang/include/clang/Format/Format.h
3076

I feel if we called it RemoveSemicolonAfterFunctionBody then adding extra use cases would then require a new option.. in the same way, RemoveBracesLLVM doesn't remove all Braces.. can we go with @owenpan suggestion of RemoveSemicolon I could go for

RemoveExtraSemicolon
RemoveExtraSemicolons

But I don't feel Extra adds anything extra, if you pardon the pune.

clang/lib/Format/UnwrappedLineParser.cpp
843

Oh! thank goodness, I hated having to add that..

922–925

I don't know why I didn't see this! much better approach

clang/unittests/Format/FormatTest.cpp
26755

I'll add the test but with @owenpan suggestion it handles it

This revision is now accepted and ready to land.Oct 8 2022, 12:54 PM

Since this option doesn't work for empty functions yet, we should either add a note in the documentation or qualify the scope with "non-empty" as suggested in my comments.

clang/docs/ReleaseNotes.rst
528

IMO "unnecessary" is unnecessary. Add "non-empty" if we want to be exact.

clang/include/clang/Format/Format.h
3058

Now I feel it's probably better to not add "redundant".

3068

Nit: unwrap the braces.

clang/lib/Format/UnwrappedLineParser.cpp
995–997

Move the comments to line 965 and elide the braces.

clang/unittests/Format/FormatTest.cpp
26773

Add the test case if it will pass.

26780–26786

And edit the comments above.

owenpan edited the summary of this revision. (Show Details)Oct 8 2022, 8:34 PM
MyDeveloperDay added a comment.EditedOct 9 2022, 2:19 AM

There is a simple fix to allow the empty functions in UnwrappedLineParser::calculateBraceTypes

by moving this clause out of the "isJavaScript()" clause this will ensure for foo() {} the { is considered a FunctionLBrace (as it will mark the {} as a BK_Block

This is having more of a knock on to other unit tests which I need to look more deeply into (as I feel they could be wrong)..

i.e.

should it be

auto i = decltype(x){}; be or auto i = decltype(x) {};

"return (a)(b){1, 2, 3};" it "return (a)(b) {1, 2, 3};"

Its possible these tests could be wrong, (some look so) by for decltype and typeof what would people expect?

Either way that change needs to be separate from this.

To work on that I have raised https://github.com/llvm/llvm-project/issues/58251

MyDeveloperDay marked 6 inline comments as done.

Minor documentation changes based on review feedback

owenpan accepted this revision.Oct 9 2022, 2:51 AM

LGTM

clang/unittests/Format/FormatTest.cpp
26773

Add the test case if it will pass.

So the test case above fails?

MyDeveloperDay marked an inline comment as done.Oct 9 2022, 2:57 AM
MyDeveloperDay added inline comments.
clang/unittests/Format/FormatTest.cpp
26773

No that passes only those in the #if 0 fail below

This revision was landed with ongoing or failed builds.Oct 9 2022, 3:19 AM
This revision was automatically updated to reflect the committed changes.
MyDeveloperDay marked an inline comment as done.