Page MenuHomePhabricator

[clang-format] [PR25010] Extend AllowShortIfStatementsOnASingleLine not working if an "else" statement is present
AbandonedPublic

Authored by MyDeveloperDay on Mar 15 2019, 2:22 AM.

Details

Summary

An addendum to D59087: [clang-format] [PR25010] AllowShortIfStatementsOnASingleLine not working if an "else" statement is present following discussions with submitter of https://bugs.llvm.org/show_bug.cgi?id=25010

it didn't make sense that, we could only do short if and not short else if and short else if they didn't have compound statments

if (true) return 0;

if (true) return 0;
else return 1;

if (true) return 0;
else if (false) return 1;

if (true) return 0;
else if (false) return 1;
else  return 2;

if (true) return 0;
else if (false) return 1;
else {
  return 2;
}

The following revision extends the capability form D59087: [clang-format] [PR25010] AllowShortIfStatementsOnASingleLine not working if an "else" statement is present to allow that if the correct setting is provided

existing true/false still honor the original meaning of AllowShortIfStatementsOnASingleLine

but the additional options now allow more fine grained control over how else/if and else should/could be handled.

Diff Detail

Event Timeline

MyDeveloperDay created this revision.Mar 15 2019, 2:22 AM
klimek added inline comments.Mar 20 2019, 4:02 AM
clang/docs/ClangFormatStyleOptions.rst
401

I'd try to make this either unconditionally what we do, or decide against doing it.

clang/include/clang/Format/Format.h
263–264

This seems weird - why would we want this?

MyDeveloperDay marked 2 inline comments as done.Mar 20 2019, 10:01 AM
MyDeveloperDay added inline comments.
clang/docs/ClangFormatStyleOptions.rst
401

see note before, this is really about maintaining compatibility, I don't want to make the assumption that everyone likes

if (x) return 1;
else if (x) return 2;
else return 3;

There could easily be users who want this.

if (x) return 1;
else if (x) return 2;
else 
   return 3;

I don't think it over complicates it, but it keeps the flexibility. The name of the option may not be the best 'suggestions welcome'

clang/include/clang/Format/Format.h
263–264

This is a compatiblity case for the previous "true" as it was before

Before

if (x) 
   return 1;
else
   return 2;

would be transformed to

if (x) return 1;
else
   return 2;

but

if (x) 
   return 1;
else {
   return 2;
}

would not be transformed at all

rdwampler added inline comments.
clang/include/clang/Format/Format.h
264

These comments can be used to auto generate the corresponding sections in clang/docs/ClangFormatStyleOptions.rst using clang/docs/tools/dump_format_style.py, so they should probably be kept in sync.

alexfh removed a reviewer: alexfh.Apr 5 2019, 5:51 AM

Generally I'm against introducing new style flags or flag options unless a supported style requires it.
IMO this increases maintenance burden and can quickly lead to a big space of style flags that contain tricky incompatibilities.

krasimir added inline comments.Apr 5 2019, 6:22 AM
clang/docs/ClangFormatStyleOptions.rst
411

nit: I prefer the term if statement instead of if/else statement.

clang/include/clang/Format/Format.h
263

ditto nit: replace If Else statements with If statements.

272

this sentence does not parse. Consider rewriting.

clang/lib/Format/UnwrappedLineFormatter.cpp
368

nit: this comparison is brittle; consider enumerating the states for which this applies explicitly.

422

nit: this comparison is brittle; consider enumerating the states for which this applies explicitly.

430

nit: this comparison is brittle; consider enumerating the states for which this applies explicitly.

Or extract a function that provides this functionality here and in other places.

clang/unittests/Format/FormatTest.cpp
520

I don't understand why is this preferred over:

if (a) f();
else
  if (b) g();

Isn't syntactically the nested if (b) g(); a compound statement? I might be wrong.

MyDeveloperDay set the repository for this revision to rC Clang.Apr 9 2019, 12:33 AM
MyDeveloperDay edited projects, added Restricted Project; removed Restricted Project.
MyDeveloperDay removed a subscriber: llvm-commits.
klimek added inline comments.Apr 9 2019, 12:48 AM
clang/docs/ClangFormatStyleOptions.rst
401

The second one seems simply broken. I think fixing this is great, but keeping the broken version seems odd :)

clang/unittests/Format/FormatTest.cpp
520

There's no compound statement, but the else-stmt is another if-stmt. That said, I don't think this is about what structure the C++ standard has (as that's not what clang-format understands anyway), but whether the structure we put in helps readability. In this case, why does the irregular break before the if help readability? It seems like we're having a break for no good reason.

MyDeveloperDay abandoned this revision.Jun 20 2019, 11:18 AM
MyDeveloperDay marked an inline comment as done.