-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Use correct line offsets for doctests #47274
Conversation
13ed204
to
82e30be
Compare
this solution is currently incorrect, ignore it for now |
c90c134
to
fe33a42
Compare
fe33a42
to
0443b7f
Compare
Ready for review. r? @QuietMisdreavus |
Need to write a test, unsure where it should go. maketest perhaps? |
src/librustdoc/test.rs
Outdated
} | ||
|
||
// Now push any outer attributes from the example, assuming they | ||
// are intended to be crate attributes. | ||
prog.push_str(&crate_attrs); | ||
line_offset += crate_attrs.lines().count(); |
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 line_offset
supposed to be the number of lines inserted by rustdoc? IIRC, crate_attrs
is actually the #![]
attributes from the example itself.
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, you're right. Given that the attrs can only be up top we should just not count this.
Nice! If i'm looking at this correctly, this will help map between the line that rustc reports an error at, and the line in the actual doctest that was responsible for it? Do we already map these lines back to the file it's in? I'm thinking about this in terms of getting the lines to work with Re: where/how to test this, i'd take a look at whether/where the regular line number reporting is tested, and seeing if you can piggyback onto that by making it call |
yeah.
We say "foobar.rs - itemname (line N)". But then the rustc error shows |
Added test. The existing stuff isn't easy to rejigger; it's built around assuming rustc. I just wrote a simple run-make test. |
0443b7f
to
44b659a
Compare
Seems good to me. Let's get this in! @bors r+ |
📌 Commit 44b659a has been approved by |
Use correct line offsets for doctests Not yet tested. This doesn't handle char positions. It could if I collected a map of char offsets and lines, but this is a bit more work and requires hooking into the parser much more (unsure if it's possible). r? @QuietMisdreavus (fixes #45868)
☀️ Test successful - status-appveyor, status-travis |
Not yet tested.
This doesn't handle char positions. It could if I collected a map of char offsets and lines, but this is a bit more work and requires hooking into the parser much more (unsure if it's possible).
r? @QuietMisdreavus
(fixes #45868)