Page MenuHomePhabricator

Fixed column shift when formatting line containing bit shift operators
ClosedPublic

Authored by idlecode on Oct 10 2016, 10:02 AM.

Details

Summary

During clang-format source lexing >> and << operators are split and
treated as two less/greater operators but column position of following
tokens was not adjusted accordingly.

Fixes pr26887

Diff Detail

Repository
rL LLVM

Event Timeline

idlecode updated this revision to Diff 74143.Oct 10 2016, 10:02 AM
idlecode retitled this revision from to Fixed column shift when formatting line containing bit shift operators.
idlecode updated this object.
idlecode added a subscriber: cfe-commits.
idlecode updated this object.Oct 10 2016, 10:16 AM
mprobst edited reviewers, added: djasper; removed: mprobst.Oct 10 2016, 10:42 AM
mprobst added a subscriber: mprobst.

Looks good to me, but Daniel is a better owner for this code.

djasper added inline comments.Oct 10 2016, 10:51 AM
test/Format/bitshift-operator-width.cpp
1 ↗(On Diff #74143)

Could you move this test into unittests/Format/FormatTest.cpp?

idlecode marked an inline comment as done.Oct 11 2016, 10:40 AM

Sure, I will upload diff as soon as I test it

idlecode updated this revision to Diff 74412.Oct 12 2016, 11:57 AM
djasper edited edge metadata.Oct 23 2016, 12:15 PM

Generally, always upload diffs with the full file as context to phabricator. That way, it is easier to see how the diff fits into the rest of the file. Thanks for fixing this!!

lib/Format/FormatTokenLexer.cpp
528 ↗(On Diff #74412)

++Column;

533 ↗(On Diff #74412)

++Column;

unittests/Format/FormatTest.cpp
11364 ↗(On Diff #74412)

Could you add this right underneath the test "UnderstandsTemplateParameters"?

11365 ↗(On Diff #74412)

It's always useful to have some other formatting being done in the same test. We repeatedly ran into cases in the past where a test only passed because some change effectively disabled formatting for a specific line. I suggest writing these as:

EXPECT_EQ("int a = 1 << 2; /* foo\n"
          "                   bar */",
          format("int    a=1<<2;  /* foo\n"
                 "                   bar */"));

Thanks for pointing it out, just a minute ago I found a proper document mentioning it (I have no idea how I could miss it).
I hope to be more use in future :)

unittests/Format/FormatTest.cpp
11365 ↗(On Diff #74412)

Oh, that is worth mentioning, thanks :)

11365 ↗(On Diff #74412)

Oh, that is good to know; Done

idlecode updated this revision to Diff 75635.Oct 24 2016, 1:08 PM
idlecode edited edge metadata.
djasper accepted this revision.Oct 24 2016, 1:11 PM
djasper edited edge metadata.

Looks good. Thank you!

This revision is now accepted and ready to land.Oct 24 2016, 1:11 PM

Do you have commit access?

No, not yet - I was about to ask someone to commit this for me :)

This revision was automatically updated to reflect the committed changes.