Repository navigation
Add position tracking when normalizing doc comments - #3430
Padmashree06 wants to merge 1 commit into
Conversation
|
@swift-ci Please test |
|
@swift-ci please test Windows |
ahoppen
left a comment
ahoppen
left a comment
There was a problem hiding this comment.
Sorry for the delayed review, @Padmashree06.
| public var docCommentValue: String? { | ||
| var comments: [Substring] = [] | ||
| var currentLineComments: [String] = [] // Reset line comments when encountering a block comment | ||
| extractDocCommentLines(tokenStart: AbsolutePosition(utf8Offset: 0))? |
There was a problem hiding this comment.
Why is passing in AbsolutePosition(utf8Offset: 0) a valid option here? Is it just because we don’t inspect DocCommentLine.position here? If so, could you explain that in a short comment?
There was a problem hiding this comment.
Yes! I have added a comment explaining it!
| var currentLineComments: [CommentEntry] = [] | ||
| var isInsideDocLineCommentSection = false | ||
| var consecutiveNewlines = 0 | ||
| var offset = 0 // UTF-8 byte offset from tokenStart, i.e. from the start of `pieces` |
There was a problem hiding this comment.
I think all we do with offset is to add it to tokenStart. Would it be clearer to have this be something like
| var offset = 0 // UTF-8 byte offset from tokenStart, i.e. from the start of `pieces` | |
| var currentPiecePosition = tokenStart | |
| … | |
| defer { currentPiecePosition += piece.sourceLength.utf8Length } |
There was a problem hiding this comment.
Yup! Modified the variable!
| // very start of `wholeText` — lines 1..n of a block are always passed through unchanged. | ||
| return comments.flatMap { entry -> [DocCommentLine] in | ||
| guard entry.wholeText.hasPrefix(prefix) else { | ||
| return entry.lines.map { DocCommentLine(text: $0.text, position: $0.position) } |
There was a problem hiding this comment.
Is the map doing anything here? Isn’t it just mapping identity?
There was a problem hiding this comment.
Yes, the map was just rebuilding the same values, it now returns entry.lines directly.
| return DocCommentLine(text: line.text, position: line.position) | ||
| } | ||
| return DocCommentLine( | ||
| text: line.text.dropFirst(prefix.count), |
There was a problem hiding this comment.
Why do we know that eg. the 2nd line here starts with prefix if we only check entry.wholeText above? I’m generally surprised that we need a distinction between the first element and any subsequent ones if we didn't need it before.
There was a problem hiding this comment.
Removed the per line loop, now we only check wholeText.hasPrefix(prefix) once and only strip lines[0], which is the start of the entry. Later lines are never checked or modified, same as before.
5584d08 to
96f7216
Compare
That's okay!! Thank you for the review!! |
4 participants
Motivation
Trivia.docCommentValuenormalizes documentation comments, but does not preserve the source position of each resulting line.DocumentationLanguageServiceneeds this mapping for features such as symbol-link diagnostics and go-to-definition, and currently maintains its own comment parsing and position tracking.This change exposes the existing normalization logic with per-line source positions so
DocumentationLanguageServicecan reuse it instead of duplicating it.Changes
Trivia.DocCommentLineto represent a normalized documentation line and its source position.Trivia.docCommentLines(startingAt:)to expose normalized lines with source positions.docCommentValueto share the same extraction logic, preserving its existing behavior.Testing
testDocCommentLinePositionstoTriviaCommentValueTests.Follow-up
Update
DocumentationLanguageServiceto usedocCommentLines(startingAt:)instead of maintaining its own comment normalization and position mapping.