This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Extend AllowShortBlocksOnASingleLine for else blocks
ClosedPublic

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

Diff Detail

Event Timeline

jessesna requested review of this revision.Nov 20 2021, 12:06 PM
jessesna created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 20 2021, 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.Nov 22 2021, 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.Nov 22 2021, 8:20 AM
MyDeveloperDay added inline comments.Nov 22 2021, 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.Nov 22 2021, 10:08 AM

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

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

I agree I think this LGTM

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

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

MyDeveloperDay added a comment.EditedNov 22 2021, 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.Nov 22 2021, 11:44 PM
This revision now requires changes to proceed.Nov 22 2021, 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.Nov 24 2021, 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.Nov 24 2021, 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.