-
Notifications
You must be signed in to change notification settings - Fork 166
Add tests for parsing of inline HTML when rendering markdown as HTML #1379
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
Add tests for parsing of inline HTML when rendering markdown as HTML #1379
Conversation
|
@swift-ci please test |
patshaughnessy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks much better now - thanks for refactoring this! With the new comments and extracted functions it's much easier for me to follow the algorithm 👍
|
|
||
| // On non-Darwin platforms, `XMLElement(xmlString:)` sometimes crashes for certain invalid / incomplete XML strings. | ||
| // To minimize the risk of this happening, don't try to parse the XML string as an empty HTML element unless it ends with "/>" | ||
| if rawHTML.hasSuffix("/>"), let parsed = try? XMLElement(xmlString: rawHTML) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you run into some crashes while running your new tests in Linux CI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| let element = elements.removeFirst() | ||
|
|
||
| guard let start = element as? InlineHTML else { | ||
| var remainder = Array(container)[...] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you're using a slice here, I imagine this is quite efficient, even though we are mutating remainder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because a slice is a portion of a buffer that's owned by another object, mutating the slice can be thought of as updating the start and end indices, which is very cheap.
If you use the Godbolt Compiler Explorer you can see that these two implementations produce rather comparable assembly code:
// Iterating over an array
func iterate(over numbers: [Int], doing work: (Int) -> Void) {
for number in numbers {
work(number)
}
}// Mutating a slice by repeatedly popping the first element
func iterate(over numbers: [Int], doing work: (Int) -> Void) {
var remainder = numbers[...]
while let number = remainder.popFirst() {
work(number)
}
}Further; if the "work" is known to the compiler, like in the comparison below, then the Swift compiler is more likely to be able to optimize away those extra retain and release calls:
// Iterating over an array
func sum(numbers: [Int]) -> Int {
var total = 0
for number in numbers {
total += number
}
return total
}// Mutating a slice by repeatedly popping the first element
func sum(numbers: [Int]) -> Int {
var total = 0
var remainder = numbers[...]
while let number = remainder.popFirst() {
total += number
}
return total
}Co-authored-by: Pat Shaughnessy <pat_shaughnessy@apple.com>
|
@swift-ci please test |
Bug/issue #, if applicable:
Summary
This is a follow up change from #1369
It adds tests for how the
DocCHTML/MarkdownRendererprocesses inline HTML—skipping any comments while preserving the rest. With these tests in place, I could refactor the implementation with more confidence and extract the inner loop into a private function.Dependencies
None.
Testing
Nothing in particular for this PR. It only adds tests and makes minor code cleanup to functionality that is only exercised in tests so far. See #1366 for how it eventually does get used.
Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/testscript and it succeeded