Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AST] Fix comment merging #39086

Merged
merged 1 commit into from
Sep 3, 2021
Merged

[AST] Fix comment merging #39086

merged 1 commit into from
Sep 3, 2021

Conversation

bnbarham
Copy link
Contributor

@bnbarham bnbarham commented Aug 28, 2021

toRawComment takes a range of line/block ordinary/doc comments and
converts them into a RawComment that should represent the doc comment
for the attached decl.

Fix a bunch of unhandled cases:

  • A prior comment with whitespace in between would cause the first
    line of the next comment to be missed
  • A gyb comment would attach the prior comment, regardless of
    whitespace inbetween

Resolves rdar://82414210

@bnbarham bnbarham requested a review from rintaro August 28, 2021 05:14
@bnbarham
Copy link
Contributor Author

@swift-ci please test

@bnbarham
Copy link
Contributor Author

bnbarham commented Aug 28, 2021

Note that there's some functionality change here - block comments are no longer merged. This makes sense to me, but let me know if you disagree.

EDIT: Ah. I missed those tests (saw test/IDE/comment_attach.swift and was running it rather than all of IDE). Do you have any thoughts here? I personally find it odd that eg.

/// Aaa.
/** Bbb. */

are merged. Even more weird that the brief comment includes both of them where as it wouldn't include Bbb in:

/// Aaa.
///
/// Bbb.

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 789d7e6

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 789d7e6

@johnfairh
Copy link
Contributor

(Could clean up SR-6605 that I, er, abandoned)

@bnbarham
Copy link
Contributor Author

(Could clean up SR-6605 that I, er, abandoned)

Oh thanks! It does indeed fix SR-6605 :). I'll also add a comment in there (I checked the source compatibility suite for instances of comments with a newline between + mixing block and line).

@bnbarham
Copy link
Contributor Author

@swift-ci please test

@bnbarham
Copy link
Contributor Author

I had a look at the source compatibility suite and there's no case where we'd want to merge blocks (mixed or otherwise) or comments with a newline in-between. Not merging is marginally better since it does fix a few cases, so I've gone ahead and modified the tests to reflect that.

So note that the behaviour is now:

  • Merge consecutive /// comments together
  • Only keep the last consecutive block of /// or the single /** */ comment (ie. do not merge block comments)

This is a little different to the Clang side, which does keep consecutive comments (including mixed), hence only the offset/length change in test/SourceKit/InterfaceGen/gen_clang_module.swift.response - the full comment is still printed, but we only keep the last one.

There's no instance of multiple block doc comments in a row or mixed block/line, so I could be convinced to keep that if anyone thinks it's worth it. Not supporting that case makes the code slightly simpler and easier to understand though.

@rintaro @johnfairh

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - aa89681

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - aa89681

@bnbarham
Copy link
Contributor Author

bnbarham commented Sep 1, 2021

I forgot to look for /// ... /**, which there are a few cases of. Guess I'll keep the mixed comment merging!

`toRawComment` takes a range of line/block ordinary/doc comments and
converts them into a `RawComment` that should represent the doc comment
for the attached decl.

Fix a bunch of unhandled cases:
  - A prior comment with whitespace in between would cause the first
    line of the next comment to be missed
  - A gyb comment would attach the prior comment, regardless of
    whitespace inbetween

Resolves rdar://82414210
@bnbarham
Copy link
Contributor Author

bnbarham commented Sep 1, 2021

@swift-ci please test

@bnbarham bnbarham merged commit 4098a16 into main Sep 3, 2021
@bnbarham bnbarham deleted the keep-comments branch September 3, 2021 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants