-
-
Notifications
You must be signed in to change notification settings - Fork 142
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
Track line offsets for better accuracy of inline sourcepos #453
Conversation
Initial spaghetti against the wall. Try using a line offset vector. It seems to solve the basic case. But small breakage in table header paragraph and the |
Run on Tue Aug 13 22:35:37 UTC 2024 |
dd04e03
to
ac4b249
Compare
@kivikakk would you be able to take a look at this when you have a moment? I think this solves one of the main issues of inline sourcepos. Or at least gets us much closer. I was able to solve the problems that we had with tables. But I haven't tackled any of the autolinking problems yet, which would be in another PR. |
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 is so good! Well done! You made this so much easier than I imagined. :)
src/cm.rs
Outdated
&& isspace(literal[literal.len() - 2]))) | ||
&& !first_in_list_item | ||
&& !self.options.render.prefer_fenced | ||
if !(!info.is_empty() |
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.
if !(!info.is_empty() | |
if !(info.len() > 0 |
Super minor, but I can't read multiple negatives (and "empty" itself is a bit negative ..).
src/parser/inlines.rs
Outdated
|
||
let node_ast = node.data.borrow(); | ||
let adjusted_line = self.line - node_ast.sourcepos.start.line; | ||
self.line_offset = *node_ast.line_offsets.get(adjusted_line).unwrap_or(&0); |
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.
Is this unwrap_or
needed, or is it purely speculative? If it's the latter, I vote for this:
self.line_offset = *node_ast.line_offsets.get(adjusted_line).unwrap_or(&0); | |
self.line_offset = node_ast.line_offsets[adjusted_line]; |
Put another way: could this mask a bug one day? Or is it actually expected that the corresponding line offset may not be found for a valid reason? I've tried this change and the unit and spec tests pass, no crashes on fuzzing, so I think we should go with it.
See also: Contracts, Undefined Behavior, and Defensive Programming.
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.
Yeah, I was being more defensive. But I think you're right, it's not needed. And if it's passing fuzzing, then I'm more confident.
I also changed it in table.rs
3b507df
to
2e144c5
Compare
Thanks @kivikakk, incorporated the changes! |
Adds a
line_offsets
vector used to record the number of spaces stripped from each line of content added. This allows us to more accurately calculatesourcepos
for inline elements.This addresses #452