const_caller_location to use real Span instead of DUMMY_SP
#146991
Conversation
|
rustbot has assigned @petrochenkov. Use |
This comment has been minimized.
This comment has been minimized.
aae6c5d to
c6a16fc
Compare
|
What does the PR title have to do with the PR contents? The PR does not link anything to any issue, as far as I can tell. |
For some reason I thought it would link. My mistake. |
|
Maybe I misunderstood what you mean by "link"... |
|
I thought that linking the issue # in the title would create a hyperlink to that issue, thus the title. |
|
I'm not talking about the use of #146990 in the title. I'm talking about the rest of the title where you say "link ... to tracking issue ...". The PR title should describe what you changed in the code, but I don't see any connection between that title and what you changed in the code. |
|
Hey @cachebag, thanks for the PR :) I think we can land this, seems simple enough. Do you want to reopen? Like @RalfJung said, we don't open issues just to link them in a PR, but that's okay. Not sure how difficult it would be, but it'd be great if we could get this change covered by a test. Though, for this, I'm not sure it's necessary. |
span_as_caller_location FIXME to tracking issue #146990DUMMY_SP
|
@RalfJung I see what you are saying, and it makes sense. A link to the issue which describes what would be changed made sense in my head, but given that GH doesn't even link in titles I understand this isn't ideal. I just changed it, thanks. @jackh726 Hi Jack, thanks for offering to keep this open for me. I can definitely see what it would take to test this. Do you know where I could start? After reading the guide fully this afternoon I can see why this PR raised eyebrows- I'll definitely make sure I follow proper contributing guidelines next time I come back here. |
|
That span is only relevant if an error occurs in this interpreter. I don't think it's possible for the interpreter to error in @oli-obk knows this code better than me though, I may be missing something. |
|
I wonder if an alternative would be to make Looking over, it seems like most uses of the Worst case, as Ralf said, can just remove the FIXME (and possibly add a comment with reasoning) |
|
Interpreters that run actual user code should have a span indicating the "root" of the evaluation. This is used in various places for error reporting. But dummy interpreters like this one don't necessarily need a span. |
|
Yea, we could remove the fixme and also just bug! in case an error occurs since it does look like it can't really fail |
70e8b82 to
4b87d94
Compare
|
Thanks for the clarifications. I reverted the span plumbing, and just replaced the FIXME with the details you all mentioned.
There's no need for another |
Co-authored-by: Ralf Jung <post@ralfj.de>
913d34a to
c98b3b2
Compare
|
Looks good, thanks. :) @bors r+ rollup |
…alfJung const_caller_location to use real Span instead of `DUMMY_SP` Closes: rust-lang#146990
Rollup of 8 pull requests Successful merges: - #146283 (Resolve: (Ref)Cell wrappers to deny mutation during spec resolution.) - #146453 (Add general arm-linux.md platform doc.) - #146991 (const_caller_location to use real Span instead of `DUMMY_SP`) - #146994 (Add `clippy::unconditional_recursion` to `./x clippy ci`) - #147027 (Add new `tyalias` intra-doc link disambiguator) - #147038 (Rename verbosity functions in bootstrap) - #147047 (rustdoc: put the toolbar on the all item index) - #147049 (std: fix warning in VEXos stdio module) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 7 pull requests Successful merges: - #146283 (Resolve: (Ref)Cell wrappers to deny mutation during spec resolution.) - #146453 (Add general arm-linux.md platform doc.) - #146991 (const_caller_location to use real Span instead of `DUMMY_SP`) - #146994 (Add `clippy::unconditional_recursion` to `./x clippy ci`) - #147038 (Rename verbosity functions in bootstrap) - #147047 (rustdoc: put the toolbar on the all item index) - #147049 (std: fix warning in VEXos stdio module) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #146991 - cachebag:span-caller-location, r=RalfJung const_caller_location to use real Span instead of `DUMMY_SP` Clarifying usage of DUMMY_SP
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#146283 (Resolve: (Ref)Cell wrappers to deny mutation during spec resolution.) - rust-lang#146453 (Add general arm-linux.md platform doc.) - rust-lang#146991 (const_caller_location to use real Span instead of `DUMMY_SP`) - rust-lang#146994 (Add `clippy::unconditional_recursion` to `./x clippy ci`) - rust-lang#147038 (Rename verbosity functions in bootstrap) - rust-lang#147047 (rustdoc: put the toolbar on the all item index) - rust-lang#147049 (std: fix warning in VEXos stdio module) r? `@ghost` `@rustbot` modify labels: rollup
Clarifying usage of DUMMY_SP