This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Add an option to remove redundant parentheses
ClosedPublic

Authored by owenpan on Jul 5 2023, 1:54 AM.

Diff Detail

Event Timeline

owenpan created this revision.Jul 5 2023, 1:54 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptJul 5 2023, 1:54 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
owenpan requested review of this revision.Jul 5 2023, 1:54 AM
MyDeveloperDay accepted this revision.Jul 5 2023, 3:35 AM

A thousand times yes!

clang/docs/ClangFormatStyleOptions.rst
4374

Should this be Leave, meaning "Don't touch anything?" vs None which could be confused with "No Parentheses"

4383

Honestly I'm fine with this as it is but should this be? to describe what it will become?

e.g. "SingleParentheses"

This revision is now accepted and ready to land.Jul 5 2023, 3:35 AM
MyDeveloperDay added a comment.EditedJul 5 2023, 10:55 AM

Here is a potential failure scenario

(test.cpp)

void *bar();

void foo() {
  void *p;
  int i = 0;

  while ((p = bar())) {
    i++;
  }
}

(.clang-format)

Language: Cpp
RemoveParentheses: ReturnStatement
clang-format test.cpp > test2.cpp

(test2.cpp)

void *bar();

void foo() {
  void *p;
  int i = 0;

  while (p = bar()) {
    i++;
  }
}
clang test2.cpp
test2.cpp:7:12: warning: using the result of an assignment as a condition
      without parentheses [-Wparentheses]
  while (p = bar()) {
         ~~^~~~~~~
test2.cpp:7:12: note: place parentheses around the assignment to silence this
      warning
  while (p = bar()) {
           ^
         (        )
test2.cpp:7:12: note: use '==' to turn this assignment into an equality
      comparison
  while (p = bar()) {
           ^
           ==
1 warning generated.
owenpan planned changes to this revision.Jul 5 2023, 2:22 PM

Yep! After reformating clang/lib/Format/ and rebuilding clang-format, I got the same warning:

WhitespaceManager.cpp:233:45: warning: using the result of an assignment as a condition without parentheses [-Wparentheses]
      } else if (Change.StartOfBlockComment = LastBlockComment) {
                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
/Users/Owen/llvm-project/clang/lib/Format/WhitespaceManager.cpp:233:45: note: place parentheses around the assignment to silence this warning
      } else if (Change.StartOfBlockComment = LastBlockComment) {
                                            ^
                 (                                            )
/Users/Owen/llvm-project/clang/lib/Format/WhitespaceManager.cpp:233:45: note: use '==' to turn this assignment into an equality comparison
      } else if (Change.StartOfBlockComment = LastBlockComment) {
                                            ^
                                            ==
1 warning generated.

I reckon you could detect the = condition between the matching paren don't you? I ran this on my quite large code base and this was one of the only issues I've seen thus far..

This is a great feature, for my codebase this generated changes in about 10% of all files (but often only a couple of lines or so per file, mostly people doing return (*this);

I think this will be a great feature to have.

clang/unittests/Format/FormatTest.cpp
25976

Should check for __attribute((what ever))__.

owenpan updated this revision to Diff 537971.Jul 6 2023, 8:55 PM

Don't remove the outermost double parentheses after if/while if there is an = inside. Also rename the options.

This revision is now accepted and ready to land.Jul 6 2023, 8:55 PM
owenpan marked an inline comment as done.Jul 6 2023, 9:26 PM

I reckon you could detect the = condition between the matching paren don't you?

Absolutely.

clang/docs/ClangFormatStyleOptions.rst
4383

Renamed it to MultipleParentheses, which means to remove redundant parentheses found in multiple (double, triple, etc.) parens. (The comment on line 3372 in Format.h says Types of redundant parentheses to remove, but it's changed to Possible values by dump_format_style.py in the documentation.) This is IMO more consistent with ReturnStatement below, but I'm open to other suggestions.

clang/unittests/Format/FormatTest.cpp
25976

What's special about this? It's agnostic about this option as it should be.

clang/unittests/Format/FormatTest.cpp
25976

I did not see, that you already had an example with __attribute__.

owenpan requested review of this revision.Jul 9 2023, 12:27 PM
This revision is now accepted and ready to land.Jul 9 2023, 1:29 PM
sstwcw added a subscriber: sstwcw.Jul 9 2023, 1:38 PM

Did you forget about GNU statement expressions?

if (({
      foo();
      bar();
    })) {
}
clang/docs/ReleaseNotes.rst
824

The option is misspelled.

owenpan updated this revision to Diff 538470.Jul 9 2023, 5:47 PM
owenpan added a reviewer: sstwcw.
owenpan removed a subscriber: sstwcw.

Don't treat ( as l_paren if it's followed by {.

owenpan marked an inline comment as done.Jul 9 2023, 5:50 PM

Did you forget about GNU statement expressions?

if (({
      foo();
      bar();
    })) {
}

Thanks!

Did you forget about GNU statement expressions?

if (({
      foo();
      bar();
    })) {
}

I wouldn't haven forgotten it, I've never heard of it.

This revision was landed with ongoing or failed builds.Jul 11 2023, 4:33 PM
This revision was automatically updated to reflect the committed changes.