This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Refactor WhitespaceManager and friends
ClosedPublic

Authored by djasper on Jan 30 2017, 1:02 PM.

Details

Summary

The main motivation behind this is to cleanup the WhitespaceManager and make it more extensible for future alignment etc. features. Specifically, WhitespaceManager has started to copy more and more code that is already present in FormatToken. Instead, I think it makes more sense to actually store a reference to each FormatToken for each change.

This has as a consequence led to a change in the calculation of indent levels. Now, we actually compute them for each Token ahead of time, which should be more efficient as it removes an unsigned value for the ParenState, which is used during the combinatorial exploration of the solution space.

No functional changes intended.

Diff Detail

Event Timeline

djasper created this revision.Jan 30 2017, 1:02 PM
klimek edited edge metadata.Jan 31 2017, 1:23 AM

Generally looks like the right direction, minus that I'm not sure yet what the plan for things broken in BreakableToken are.

lib/Format/TokenAnnotator.cpp
1843

Perhaps add an assert that we don't underflow.

lib/Format/UnwrappedLineFormatter.cpp
904–908

I'm not sure I understand the change in the function signature. Given that we really only need InPPDirective and FirstToken, it seems unnecessary to hand in the whole line? (in the spirit of minimal interfaces)

lib/Format/WhitespaceManager.h
109–110

What's the proposed fix?

djasper marked an inline comment as done.Jan 31 2017, 1:38 AM
djasper added inline comments.
lib/Format/UnwrappedLineFormatter.cpp
904–908

I think this interface makes more sense:

  • I don't think it's significantly "larger" as we already have access to the whole previous line.
  • Having a function that's called formatFirstToken, but can actually be called with an arbitrary token seems weird.
lib/Format/WhitespaceManager.h
109–110

Removed InToken (added that at first, but found the other one later).

I don't have a proposed fix. I just moved this comment from the original line 122.

This revision is now accepted and ready to land.Jan 31 2017, 3:24 AM
djasper updated this revision to Diff 86404.Jan 31 2017, 3:25 AM

Added assert. Removed unused InToken.

djasper closed this revision.Jan 31 2017, 3:37 AM

Submitted as r293616.

krasimir added inline comments.Jan 31 2017, 4:43 AM
lib/Format/WhitespaceManager.cpp
178

What happened to the tok::unknown case?

krasimir edited edge metadata.Jan 31 2017, 4:44 AM

looks good!

djasper marked an inline comment as done.Jan 31 2017, 4:45 AM
djasper added inline comments.
lib/Format/WhitespaceManager.cpp
178

We actually don't set the token back to unknown in replaceWhitespaceInToken anymore if this is a comment. So I moved this logic back into the if above.