Fixes 'unresolved link to ...' when brackets are present in comments #1956
Conversation
There was a problem hiding this comment.
So I'm not totally opposed to this, but if we really want to fix rustdoc's interactions with bindgen, that's a quite large task, see also #1313 or such...
I kinda feel that patching one issue at a time would be mostly just working around it.
I think a better option could be to just wrap all doc comments bindgen emits into triple-backticks, effectively, and then escape those from the comment content?
I think that'd fix what you're hitting and pretty much all those related issues, plus it would more generally preserve the C comment style, which is not expected to be parsed as markdown (rustdoc messes up all sorts of ascii art or what not).
I suspect it should also not be much implementation work, wdyt?
src/ir/comment.rs
Outdated
| /// | ||
| /// Avoid processing expensive computations if no brackets are found | ||
| fn preprocess_plain_brackets(line: &str) -> String { | ||
| if line.contains("[") && line.contains("]") { |
There was a problem hiding this comment.
Let's not do two scans of the string?
src/ir/comment.rs
Outdated
| /// Avoid processing expensive computations if no brackets are found | ||
| fn preprocess_plain_brackets(line: &str) -> String { | ||
| if line.contains("[") && line.contains("]") { | ||
| line.chars() |
There was a problem hiding this comment.
I kinda feel this would be more readable as a regular for loop, but maybe it's just me so if you feel strongly against that no big deal :)
src/ir/comment.rs
Outdated
| /// Preprocesses comments containing brackets to prevent `unresolved link to ...` | ||
| /// | ||
| /// Avoid processing expensive computations if no brackets are found | ||
| fn preprocess_plain_brackets(line: &str) -> String { |
There was a problem hiding this comment.
Let's return a Cow<str> instead and avoid cloning the string if there are no brackets.
|
So, to sum up, what you said, you think that making the whole comment block become Markdown, by prepending and appending it with triple-backticks, which would be easier to escape actual backticks in the sequence (String::replace maybe?). Did I get it right? Besides that, my fix was supposed to be the least intrusive possible, in order not to break the tests in place, what you meant is that we should re-write tests to implement this fix. |
Yeah, I think that's right.
Yeah, tests should be automatically rewritten (the ones that test generated output, that is) by using |
Merge back commits from master
|
Sorry for the lag. This looks generally good, it seems CI is red because probably expectations for older libclang versions need adjusting. Let me know if you don't have the cycles to finish it up and I can try to pick this up. Thanks for the patch! |
|
What would be needed in order to adjust the libclang so that the CI goes green ? |
|
Sorry, I missed that comment. The CI run in https://github.com/rust-lang/rust-bindgen/runs/1624844280 prints the diffs of the tests that failed, so far it seems like a matter of changing the test expectation in this case. |
|
☔ The latest upstream changes (presumably 0e25962) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Closing this at the same can be done by providing an implementation of |
Related to #1955