This is an archive of the discontinued LLVM Phabricator instance.

Refactor ContinuationIndenter's breakProtrudingToken logic into slightly more orthogonal pieces.
ClosedPublic

Authored by klimek on Nov 10 2017, 6:56 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

klimek created this revision.Nov 10 2017, 6:56 AM
krasimir added inline comments.Nov 10 2017, 7:25 AM
lib/Format/ContinuationIndenter.cpp
1028 ↗(On Diff #122432)

Why handleEndOfLine? Is it guaranteed that here we've reached the end of the line?

1284 ↗(On Diff #122432)

At this point, we compute getRawStringDelimiter twice: once here, and once in getRawStringStyle in the caller side. Ideally, we'd only like to compute it once. That's why it was a parameter before.

1408 ↗(On Diff #122432)

If AllowBreak == false, in which cases will this return a non-nullptr?

unittests/Format/FormatTest.cpp
6322 ↗(On Diff #122432)

What happened to the old test line?

unittests/Format/FormatTestComments.cpp
683 ↗(On Diff #122432)

Please also add a test case adding a 6, which actually gets broken.

klimek updated this revision to Diff 122433.Nov 10 2017, 7:36 AM
klimek marked an inline comment as done.
  • Add test.

Updating D39900: Refactor ContinuationIndenter's breakProtrudingToken logic into slightly more

orthogonal pieces.

lib/Format/ContinuationIndenter.cpp
1028 ↗(On Diff #122432)

No, this is used to do things in case we have reached the end of the line, as that's hard to check. I also wanted a better name, but wasn't able to come up with one (and we do similar things in other places here, where checking a condition is equivalent to running through all the code to do the things necessary).

1284 ↗(On Diff #122432)

I got that, but I think passing around the delimiter is super awkward, and computing it is cheap. If computing it is the problem we should store it in the FormatToken.

1408 ↗(On Diff #122432)

Only for string literals, as AllowBreak is really about whether we can break in the token space, but for comments (or raw strings, but those are handled differently anyway) we break in the comment space.

unittests/Format/FormatTest.cpp
6322 ↗(On Diff #122432)

It was unnecessarily long. I found it weird that it went out of its way to add more a's over the line limit.

krasimir accepted this revision.Nov 10 2017, 8:50 AM

Maybe we should further refactor getRawStringStyle into llvm::Optional<std::pair<style, delimiter>> getRawStringStyleAndDelimiter and that would nicely take care of the duplicated effort?

This revision is now accepted and ready to land.Nov 10 2017, 8:50 AM

Maybe we should further refactor getRawStringStyle into llvm::Optional<std::pair<style, delimiter>> getRawStringStyleAndDelimiter and that would nicely take care of the duplicated effort?

I thought about that, too, but I'm still convinced the right solution if we really care about the runtime is to cache the result in FormatToken itself.

This revision was automatically updated to reflect the committed changes.