This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Break before a sequence of line comments aligned with the next line.
ClosedPublic

Authored by krasimir on Feb 7 2017, 2:48 AM.

Details

Summary

Make the comment alignment respect sections of line comments originally alinged
with the next token. Until now the decision how to break a continuous sequence
of line comments into sections was taken without reference to the next token.

source:

class A {
public: // comment about public
  // comment about a
  int a;
}

format before:

class A {
public: // comment about public
        // comment about a
  int a;
}

format after:

class A {
public: // comment about public
  // comment about a
  int a;
}

Event Timeline

krasimir created this revision.Feb 7 2017, 2:48 AM
krasimir edited the summary of this revision. (Show Details)Feb 7 2017, 2:50 AM
krasimir added a reviewer: djasper.
krasimir added a subscriber: cfe-commits.

I feel that this is a bit too rough now; any suggestions on improving the architecture/design of this patch are welcome!

lib/Format/UnwrappedLineParser.cpp
2264

Need to add comments about this if we decide we may go with this.

2265

Any suggestions on how to improve the code quality a bit if we stick with this design?

lib/Format/UnwrappedLineParser.h
118

@djasper: considering what this code does, what would be a better name of this?

djasper added inline comments.Feb 7 2017, 3:04 AM
lib/Format/UnwrappedLineParser.cpp
2264

Yeah.. Commented comment handling is best comment handling :-D

2265

I think comments might actually help here, but also this isn't too bad. As in, when writing the comments, it might become more obvious how to write this better.

2267

I think we use "unsigned" in most of these loops.

And to avoid and edge conditions put this at the top of the function:

if (Comments.empty())
  return;
lib/Format/UnwrappedLineParser.h
118

Maybe analyzeCommentAlignment? Or determineCommentAlignment?

klimek added a comment.Feb 7 2017, 3:10 AM

I think this looks pretty good. More comments would help :) Also, organize is spelled with a 'z' in American.

krasimir updated this revision to Diff 87410.Feb 7 2017, 6:04 AM
krasimir marked 6 inline comments as done.
  • Address review comments.
krasimir marked an inline comment as done.Feb 7 2017, 6:04 AM
krasimir edited the summary of this revision. (Show Details)Feb 7 2017, 6:07 AM
klimek added inline comments.Feb 7 2017, 6:26 AM
lib/Format/UnwrappedLineParser.cpp
2207–2208

Put these 2 lines and what it does to the arguments into the header.

krasimir updated this revision to Diff 87424.Feb 7 2017, 6:59 AM
  • Moved method summary from the implementation to the declaration.
krasimir marked an inline comment as done.Feb 7 2017, 7:00 AM
krasimir added inline comments.
lib/Format/UnwrappedLineParser.cpp
2207–2208

Done. Thanks!

klimek added inline comments.Feb 7 2017, 7:02 AM
lib/Format/UnwrappedLineParser.h
121–123

Given this, I'd perhaps call this addSection or addCommentSection or something similar? analyze sounds like it doesn't change the state of the class...

krasimir marked an inline comment as done.Feb 8 2017, 12:08 AM
krasimir added inline comments.
lib/Format/UnwrappedLineParser.h
121–123

Maybe distributeCommentsAccordingToAlignment?

klimek added inline comments.Feb 8 2017, 1:11 AM
lib/Format/UnwrappedLineParser.h
121–123

I don't think "AccordingToAlignment" needs to be in the name - it's fine that that's in the comment, and it's kind of a detail of what this does.

The important thing at the call site is that this adds comments to the current unwrapped line, right?

krasimir added inline comments.Feb 8 2017, 1:34 AM
lib/Format/UnwrappedLineParser.h
121–123

That's just half of it: it may add comments to the current line, and it may add comments to the sequence of tokens preceding the next token with conceptually belong to the next token and will be put in a new line if the next token has to be put on a new line.
After a while, there is "flushComments" which takes care of the mentioned comments before next token.

klimek added inline comments.Feb 8 2017, 1:38 AM
lib/Format/UnwrappedLineParser.h
121–123

Sure, but the important part seems to be that it decides which comment tokens belong into the current line?

krasimir updated this revision to Diff 87614.Feb 8 2017, 1:49 AM
  • Invent better name for comment analysis.
krasimir updated this revision to Diff 87615.Feb 8 2017, 1:53 AM
krasimir marked an inline comment as done.
  • Updated call sites.
lib/Format/UnwrappedLineParser.h
121–123

Renamed. While at it, I added a line of comments for "flushComments" since now we have two methods with similar enough names to cause a little confusion with no comments.

klimek accepted this revision.Feb 8 2017, 2:17 AM

lg.

I'd probably still call it consumeComments or something, as we require a flush afterwards for the unconsumed comments, but I don't feel too strongly about that.

This revision is now accepted and ready to land.Feb 8 2017, 2:17 AM
This revision was automatically updated to reflect the committed changes.