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

Jump to bottom

[SR-6605] Doc comment merging issues #49155

Closed
johnfairh opened this issue Dec 13, 2017 · 4 comments
Closed

[SR-6605] Doc comment merging issues #49155

johnfairh opened this issue Dec 13, 2017 · 4 comments
Assignees
@bnbarham
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior.

Comments

@johnfairh
Copy link
Contributor

Previous ID SR-6605
Radar rdar://82414210
Original Reporter @johnfairh
Type Bug
Status Resolved
Resolution Done
Environment

Xcode 9.2

Additional Detail from JIRA
Votes 0
Component/s Source Tooling
Labels Bug
Assignee @bnbarham
Priority Medium

md5: 0e37b8ffa8a381dbc5e9566da25b0955

Issue Description:

Handling of doc comments that should not be merged is a bit wrong.

First problem:

/** one */

/** two */
let v = 1

...gives an empty doc comment. Per the intent of the code it should give ‘two’. (Maybe someone wants to argue for changing to ‘one two’ but empty has to be wrong.)

Second problem:

/// one

/// two
let w =1

...gives a doc comment of ‘one two’. Changing the spacing to two blank lines gives an empty doc comment again.

I’ll try a PR to fix both parts. I’m conscious that code might be using the second behavior today - the two instances I found in swift though look as though they would be better without merging:
swiftpm swift/stdlib
@krilnon
Copy link
Member

krilnon commented Dec 13, 2017

@natecook1000 probably has thoughts on this. How did you search/grep for the potential impact of this change?

@johnfairh
Copy link
Contributor Author

How did you search?

'poorly' basically – with a better regex today:

find . \( -name '*.swift' -or -name '*.swift.gyb' \) -type f -exec pcregrep -HM ^\\s*///.*?\\n\\s*\\n\\s*/// {} \+

Real list in the swift project is:

These are all marginally improved by fixing /// merging; there are no places intentionally taking advantage of it. Scanning a handful of open source projects turned up one hit, another 'MARK' typo.

@bnbarham
Copy link
Contributor

I had a look at the source compatibility suite and there's no case where we want to merge:

  • Block comments together (mixed or otherwise)
  • Comments with a newline in-between

There's a few cases where not merging is better though, so I've fixed the empty case and prevented block comments merging altogether in #39086

EDIT: I forgot to look for `/// ... /**` - there are a few cases of that (all in `RxSwift`). Guess I'll keep the merging of mixed comments!

@bnbarham
Copy link
Contributor

bnbarham commented Sep 3, 2021

Resolved in #39086

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bnbarham bnbarham

Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@krilnon @bnbarham @johnfairh