Page MenuHomePhabricator

[clang-format] [PR41170] Break after return type ignored with certain comments positions
ClosedPublic

Authored by MyDeveloperDay on Apr 6 2019, 7:26 AM.

Details

Summary

Addresses https://bugs.llvm.org/show_bug.cgi?id=41170

The AlwaysBreakAfterReturn type setting can go wrong if the line ends with a comment

void foo() /* comment */

or

void foo() // comment

It will incorrectly see such functions as Declarations and not Definitions

The following code addresses this by looking for function which end with ; <comment> rather than just ; or <comment>

Diff Detail

Repository
rL LLVM

Event Timeline

MyDeveloperDay created this revision.Apr 6 2019, 7:26 AM
owenpan requested changes to this revision.Apr 7 2019, 3:19 PM
owenpan added inline comments.
clang/lib/Format/TokenAnnotator.h
114–116 ↗(On Diff #194021)

May I suggest the following instead?

return !endsWith(tok::semi);
This revision now requires changes to proceed.Apr 7 2019, 3:19 PM
owenpan added a subscriber: sammccall.

I added @sammccall to the reviewer list as I'm not sure if we need all these test cases or if there are overlaps among them. It seems all we need is a test case that has a function definition which has a comment after the closing parenthesis of the function header, e.g.:

void
foo() // this might be a function definition
{
}
klimek added a comment.Apr 8 2019, 1:13 AM

+1 to quite some overlap in the tests. I'd have expected three test cases:

  1. ; line comment
  2. ; C-comment
  3. no semi

+1 to quite some overlap in the tests. I'd have expected three test cases:

  1. ; line comment
  2. ; C-comment
  3. no semi

Yes, sorry about that.. the tests were left over from my debugging investigation, I originally thought the bug was caused by the comment in the arguments and not by the comment after the ;, I'll update the patch to remove necessary test cases and resubmit

Addressing review comments
Reduce test cases

owenpan added inline comments.Apr 8 2019, 1:04 PM
clang/lib/Format/TokenAnnotator.h
114 ↗(On Diff #194120)

I think endsWith() ignores tok::comment, so

return !endsWith(tok::semi);

should suffice?

MyDeveloperDay marked an inline comment as done.Apr 9 2019, 12:25 AM
MyDeveloperDay added inline comments.
clang/lib/Format/TokenAnnotator.h
114 ↗(On Diff #194120)

I should ready the code comments more often! thanks, I'll update

lebedev.ri set the repository for this revision to rC Clang.Apr 9 2019, 12:27 AM
lebedev.ri edited projects, added Restricted Project; removed Restricted Project.
lebedev.ri edited subscribers, added: cfe-commits; removed: llvm-commits.
lebedev.ri added a subscriber: lebedev.ri.

Please either subscribe the correct lists or set the correct repo in differential params.

Please either subscribe the correct lists or set the correct repo in differential params.

Thanks for the correction, I was never really clear which one to use for the project as the blurb at the top of clang-tools-extra says "clang-format"

https://reviews.llvm.org/tag/clang-tools-extra/

But going forward I'll follow your example here.

use endsWith() as it ignored comments

MyDeveloperDay marked 2 inline comments as done.Apr 14 2019, 9:07 AM
owenpan accepted this revision.Apr 14 2019, 11:33 AM

Looks good!

This revision is now accepted and ready to land.Apr 14 2019, 11:33 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2019, 12:45 AM