[PATCH] D135466: [clang-format] Add support to remove unnecessary semicolons after function definition

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

@MyDeveloperDay Cool!

In D135466#3843792 <https://reviews.llvm.org/D135466#3843792>, @HazardyKnusperkeks wrote:

> 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`.



================
Comment at: clang/docs/ClangFormatStyleOptions.rst:3761-3762
 
+**RemoveSemiColons** (``Boolean``) :versionbadge:`clang-format 16`
+  Removes unnecessary semicolons from the function braces
+
----------------
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.


================
Comment at: clang/docs/ClangFormatStyleOptions.rst:3770
+   NOTE:
+   Setting this to false will not add `;` where they were missing
+
----------------
IMO we don't need the note as it goes without saying.


================
Comment at: clang/docs/ClangFormatStyleOptions.rst:3779
+      return a>b?a:b;                            return a>b?a:b;
+    };                                         };
+
----------------
lahwaacz wrote:
> There is an extra `;` in the `true` case ;-)
> 
If this file was generated from the Format.h header below, we have a bug in the dump_format_style.py script.


================
Comment at: clang/include/clang/Format/Format.h:3069-3071
+  ///   int max(int a, int b)                      int max(int a, int b)
+  ///   {                                          {
+  ///     return a>b?a:b;                            return a>b?a:b;
----------------
LLVM style.


================
Comment at: clang/include/clang/Format/Format.h:3076
+  /// \version 16
+  bool RemoveSemiColons;
+
----------------
HazardyKnusperkeks wrote:
> 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`?
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`.


================
Comment at: clang/lib/Format/Format.cpp:1971
+          continue;
+        if (!Token->is(tok::semi))
+          continue;
----------------
Nit.


================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:843
+    bool CanContainBracedList, TokenType NextLBracesType,
+    bool IsFunctionBlock) {
   auto HandleVerilogBlockLabel = [this]() {
----------------
We don't need to add the parameter. See below.


================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:923
   }
 
   auto RemoveBraces = [=]() mutable {
----------------
So that we don't need to add a parameter to `parseBlock`.


================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:963
   nextToken(/*LevelDifference=*/-AddLevels);
   HandleVerilogBlockLabel();
 
----------------
The `while` loop would address https://reviews.llvm.org/D135466#inline-1306331 but is optional IMO because it wouldn't distinguish `};;` and the following:
```
};
;
```


================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:986-990
+    if (IsFunctionBlock) {
+      FormatToken *Previous = Tokens->getPreviousToken();
+      if (Previous && Previous->is(tok::r_brace))
+        FormatTok->Optional = true;
+    }
----------------
This can be deleted. See above. Now we don't have to worry about if `parseParens()` and `parseStructuralElement()` would see a `};` sequence.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135466/new/

https://reviews.llvm.org/D135466