Page MenuHomePhabricator

[clang-format] Extend AllowShortBlocksOnASingleLine for else blocks
ClosedPublic

Authored by jessesna on Sat, Nov 20, 12:06 PM.

Diff Detail

Event Timeline

jessesna requested review of this revision.Sat, Nov 20, 12:06 PM
jessesna created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptSat, Nov 20, 12:06 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
MyDeveloperDay added a project: Restricted Project.
MyDeveloperDay removed a subscriber: llvm-commits.

Could you add the case where the else is not empty.

clang/unittests/Format/FormatTest.cpp
5413

Can you please use verifyFormat (as this does some extra stuff to ensure its stable)

Nothing majorly wrong here, its looks pretty good.

jessesna updated this revision to Diff 388924.Mon, Nov 22, 8:18 AM

use verifyFormat instead of EXPECT_EQ (as this does some extra stuff to ensure its stable)

jessesna marked an inline comment as done.Mon, Nov 22, 8:20 AM
MyDeveloperDay added inline comments.Mon, Nov 22, 9:48 AM
clang/unittests/Format/FormatTest.cpp
5419

you can remove the "format(" here now..."

when its 2 arguments to verifyFormat its a kind of verifyFormat(after,before,Style)

alternatively you can use:

verifyFormat(after,Style)

it will deliberately messUp the the text and ensure it resets itself back to the desired after

jessesna updated this revision to Diff 388961.Mon, Nov 22, 10:08 AM

use the more dense "verifyFormat(after,Style)" calls

jessesna marked an inline comment as done.Mon, Nov 22, 10:09 AM
This revision is now accepted and ready to land.Mon, Nov 22, 1:29 PM
MyDeveloperDay accepted this revision.Mon, Nov 22, 2:56 PM

I agree I think this LGTM

owenpan accepted this revision.Mon, Nov 22, 8:27 PM

Should we add an if-else example to the documentation?

MyDeveloperDay added a comment.EditedMon, Nov 22, 11:43 PM

Ok now I'm a little puzzled, shouldn't this be covered by AllowShortIfStatementsOnASingleLine?

I think at a minimum we need to understand why those options don't work, and I agree we should add a ReleaseNote and add something to the documentation (by editing Format.h)

MyDeveloperDay requested changes to this revision.Mon, Nov 22, 11:44 PM
This revision now requires changes to proceed.Mon, Nov 22, 11:44 PM

Ok now I'm a little puzzled, shouldn't this be covered by AllowShortIfStatementsOnASingleLine?

That's exactly what i thought when i first stumbled over the behavior :D But I think the majority of users might not even consider adding empty else blocks. Only a few either just do it or have to comply with some standards, f.e. MISRA .

MyDeveloperDay accepted this revision.Wed, Nov 24, 11:43 AM

Ok now I'm a little puzzled, shouldn't this be covered by AllowShortIfStatementsOnASingleLine?

That's exactly what i thought when i first stumbled over the behavior :D But I think the majority of users might not even consider adding empty else blocks. Only a few either just do it or have to comply with some standards, f.e. MISRA .

I tend to agree with you.

This revision is now accepted and ready to land.Wed, Nov 24, 11:43 AM

Thank you for the patch, Do you need help committing this? if so we need your name and email address to do so

https://llvm.org/docs/DeveloperPolicy.html#commit-messages

Thank you for the patch, Do you need help committing this? if so we need your name and email address to do so

https://llvm.org/docs/DeveloperPolicy.html#commit-messages

Thanks for the reminder. I wasn't sure about where we are in the process. I just did a rebase, compiled and run the tests again and it's all still fine but this is my first contribution and i don't think i have commit access according to https://www.llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access . I'd be glad if someone else could commit for me. What's the best method to send the email address?

one of us can commit it for you but we need to do the following to attribute it to you

commit --amend --author="John Doe <jdoe@llvm.org>"

So I need your name and email address.

Great. Thanks a lot. It's Jesses Gott and jesses.gott.na+llvm@gmail.com

Thanks a lot for your time and help @MyDeveloperDay

Thanks a lot for your time and help @MyDeveloperDay

No problem, come play some more! We need people to help and review.