Page MenuHomePhabricator

clang-format: Extend AllowShortLoopsOnASingleLine to do ... while loops.
ClosedPublic

Authored by DaanDeMeyer on Feb 23 2020, 11:03 AM.

Details

Summary
do
  a++;
while (true);

becomes

do a++;
while (true);

if AllowShortLoopsOnASingleLine is enabled.

One of the projects where I want to introduce clang-format formats single-line statements following a do on the same line as the do statement. This is one of the blockers in introducing clang-format for this project. I'm not sure if it's fine to put this behind an existing option or if a new option (AllowShortDoWhileStatementsOnASingleLine) should be introduced for this.

This is my first contribution to clang-format so apologies if I missed anything obvious. Any comments or help with any further steps to get this merged would be appreciated.

Diff Detail

Event Timeline

DaanDeMeyer created this revision.Feb 23 2020, 11:03 AM
Eugene.Zelenko edited reviewers, added: MyDeveloperDay; removed: Restricted Project.Feb 23 2020, 1:56 PM
Eugene.Zelenko set the repository for this revision to rG LLVM Github Monorepo.
Eugene.Zelenko added a subscriber: cfe-commits.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 23 2020, 1:56 PM

You need to add unit tests

MyDeveloperDay requested changes to this revision.Feb 24 2020, 4:02 AM
This revision now requires changes to proceed.Feb 24 2020, 4:02 AM
DaanDeMeyer updated this revision to Diff 246244.EditedFeb 24 2020, 10:08 AM

Added unit tests and fixed the case where stuff follows the do statement (like a comment).

I think its look good, please add the extra tests, then lets give people a couple of days

unittests/Format/FormatTest.cpp
570 ↗(On Diff #246244)

horrible code though they are, could you add a test for the label and comment case

do
label:
a++
white(true);

and

do
// comment
a++
white(true);

Added extra unit tests

mitchell-stellar accepted this revision.Feb 26 2020, 6:20 AM

Assuming this passes all existing tests, LGTM.

Is there CI infra that runs for each revision? I verified all the format unit tests still pass but I haven't run the entire test suite on my machine.

Not that I am aware of. Whoever ends up doing the merge will likely run the necessary tests before committing. If you've run as many as you can, then hopefully all will be fine.

MyDeveloperDay accepted this revision.Feb 28 2020, 12:51 AM

LGTM, thank you for adding the extra test, please mark the inline comments as done

This revision is now accepted and ready to land.Feb 28 2020, 12:51 AM
DaanDeMeyer marked an inline comment as done.Feb 28 2020, 1:12 AM
This revision was automatically updated to reflect the committed changes.