Page MenuHomePhabricator

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

Authored by MyDeveloperDay on Mar 7 2019, 3:30 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

MyDeveloperDay created this revision.Mar 7 2019, 3:30 AM

Add missing Format.h from the review

Higuoxing added inline comments.
clang/include/clang/Format/Format.h
247 ↗(On Diff #189837)

Small nit:

AllowShortIfIfStatementsOnASingleLine
            ^

Fix spelling typo in documentation and comment

MyDeveloperDay marked an inline comment as done.Mar 8 2019, 6:51 AM
reuk added a subscriber: reuk.Mar 10 2019, 5:50 AM

Does it make sense to allow AllowShortIfElseStatementsOnASingleLine to be enabled when AllowShortIfStatementsOnASingleLine is not?

Perhaps it would be better to have AllowShortIfStatementsOnASingleLine be represented by an enum with values Never, WithoutElse, Always. The old true/false values from config files prior to this change would need to be made to map to the appropriate enum keys.

MyDeveloperDay planned changes to this revision.Mar 10 2019, 5:59 AM

Does it make sense to allow AllowShortIfElseStatementsOnASingleLine to be enabled when AllowShortIfStatementsOnASingleLine is not?

Perhaps it would be better to have AllowShortIfStatementsOnASingleLine be represented by an enum with values Never, WithoutElse, Always. The old true/false values from config files prior to this change would need to be made to map to the appropriate enum keys.

Thats a good point...its probably more code but I think it might be a better approach

Address review comments

  • collapse two options into one
reuk added a comment.Mar 11 2019, 2:01 AM

This looks much better now, thanks for taking another look! I've flagged some formatting/spelling nits, and I think the tests/docs could be a little more explicit about the behaviour of WithoutElse. Other than that, looks great!

clang/docs/ClangFormatStyleOptions.rst
375 ↗(On Diff #190049)

For consistency, these descriptions should end with periods .

386 ↗(On Diff #190049)

Missing .

390 ↗(On Diff #190049)

I think an example of the generated formatting *with* a compound else statement would be useful here too, in addition to the existing example.

395 ↗(On Diff #190049)

compounf -> compound

395 ↗(On Diff #190049)

Missing .

clang/include/clang/Format/Format.h
264 ↗(On Diff #190049)

Missing .

clang/lib/Format/UnwrappedLineFormatter.cpp
417 ↗(On Diff #190049)

Has this line been clang-formatted? I'd expect spaces around the binop

clang/unittests/Format/FormatTest.cpp
494 ↗(On Diff #190049)

I think having a test here which shows the expected behaviour for non-compound else statements with WithoutElse would be helpful, as in the documentation.

MyDeveloperDay marked 8 inline comments as done.

Address review comment

  • add document and comment spelling and punctuation
  • add additional unit tests to show non compound else clause case
reuk added a comment.Mar 11 2019, 1:39 PM

The code looks good now. There's just a few places left where the formatting looks a bit suspect (sorry for missing those last time). I think once that's fixed this will be good to go.

clang/unittests/Format/FormatTest.cpp
553 ↗(On Diff #190053)

This line, along with some of the following changed lines, have more than 80 characters. Make sure to clang-format changes! Consider using git-clang-format to format just this patch.

run git clang-format

reuk accepted this revision.Mar 12 2019, 3:41 PM

LGTM

This revision is now accepted and ready to land.Mar 12 2019, 3:41 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald Transcript
MyDeveloperDay reopened this revision.Mar 13 2019, 1:19 AM
This revision is now accepted and ready to land.Mar 13 2019, 1:19 AM
This revision was automatically updated to reflect the committed changes.

I believe there is no such thing as an "short else statement". The else is part of the if statement and if it is present, I don't consider the whole if statement short. As such, IMO the bug is invalid.

klimek added inline comments.Apr 8 2019, 1:55 AM
cfe/trunk/docs/ClangFormatStyleOptions.rst
384

Is that actually the status quo or is the current behavior missing?

MyDeveloperDay marked 2 inline comments as done.Apr 8 2019, 2:08 AM

I believe there is no such thing as an "short else statement". The else is part of the if statement and if it is present, I don't consider the whole if statement short. As such, IMO the bug is invalid.

I understand your point, but that is just one viewpoint only.

The opinion of the person who logged the defect (and I'm not that user just someone who wants to address the defects), whether something is a bug or not is 100% about their perspective, (that doesn't mean we have to address everything)

The question is does the change to the option hurt anyone who doesn't care? Is it useful the to the user requesting it? Does it significantly impact our ability to maintain the code, could it be useful to others.

cfe/trunk/docs/ClangFormatStyleOptions.rst
384

Yes that is "as was"

MyDeveloperDay marked an inline comment as done.Apr 8 2019, 2:08 AM
klimek added inline comments.Apr 8 2019, 3:23 AM
cfe/trunk/docs/ClangFormatStyleOptions.rst
384

Ok, that seems... wrong :)
Specifically, I'd at least expect the first one to also do

if (a) return;
else return;

That would be SIS_Always now? If so, could we make "true" just be "Always"?