-
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
Improve Span in smir #115772
Improve Span in smir #115772
Conversation
This PR changes Stable MIR cc @oli-obk, @celinval, @spastorino |
@@ -139,6 139,10 @@ impl<'tcx> Tables<'tcx> { | |||
stable_mir::ty::Prov(self.create_alloc_id(aid)) | |||
} | |||
|
|||
pub fn span(&mut self, span: Span) -> stable_mir::ty::Span { | |||
self.create_span(span) |
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 could be the body of a Stable
impl, so instead of removing the impl, put this logic into it.
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.
You can remove the span
function now, it's not needed anymore
fn span_of_an_item(&mut self, def_id: stable_mir::DefId) -> stable_mir::ty::Span { | ||
self.span(self.tcx.def_span(self[def_id])) | ||
fn span_of_an_item(&mut self, def_id: stable_mir::DefId) -> String { | ||
opaque(&self.tcx.def_span(self[def_id])).to_string() |
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.
Sorry. The confusion was my inability to use github correctly.
Lemme try again:
With this change, there is no way to obtain a span anymore. Before this PR this function returned Opaque
, now it returns String
, which is the same thing.
If I understand it correctly, this commit is because you were unable to meaningfully debug print Span
. If you wanna fix that, I think, instead of this commit, you should do something similar to Span
's Debug
impl as was done to DefId
's Debug
impl
self.tcx.def_span(self[def_id]).stable(self) | ||
fn span_of_an_item(&mut self, def_id: stable_mir::DefId) -> Span { | ||
self.span(self.tcx.def_span(self[def_id])) |
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.
keep using the stable
method for consistency
impl<'tcx> Stable<'tcx> for rustc_span::Span { | ||
type T = stable_mir::ty::Span; | ||
|
||
fn stable(&self, _: &mut Tables<'tcx>) -> Self::T { | ||
// FIXME: add a real implementation of stable spans | ||
opaque(self) | ||
} | ||
} | ||
|
||
impl<T> From<ErrorGuaranteed> for CompilerError<T> { | ||
fn from(_error: ErrorGuaranteed) -> Self { | ||
CompilerError::CompilationFailed | ||
} | ||
} | ||
|
||
impl<'tcx> Stable<'tcx> for rustc_span::Span { | ||
type T = stable_mir::ty::Span; | ||
|
||
fn stable(&self, tables: &mut Tables<'tcx>) -> Self::T { | ||
tables.span(*self) | ||
} | ||
} |
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.
move the impl back up to where it was to keep the diff simpler
@@ -160,7 160,7 @@ pub trait Context { | |||
/// Prints the name of given `DefId` | |||
fn name_of_def_id(&self, def_id: DefId) -> String; | |||
|
|||
/// `Span` of an item | |||
/// `Span` of an item in `Opaque` form. |
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.
not anymore 😆
pub fn get_rustc_span(span: stable_mir::ty::Span) -> rustc_span::Span { | ||
with_tables(|t| t.spans[span.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.
inline this function in its one use site, just like we did with DefId
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.
Please squash all commits once the review comments have been addressed
39116f1
to
549b677
Compare
@bors r rollup |
Improve Span in smir Addressing rust-lang/project-stable-mir#31 r? `@oli-obk`
…iaskrgr Rollup of 4 pull requests Successful merges: - rust-lang#115772 (Improve Span in smir) - rust-lang#115832 (Fix the error message for `#![feature(no_coverage)]`) - rust-lang#115834 (Properly consider binder vars in `HasTypeFlagsVisitor`) - rust-lang#115844 (Paper over an accidental regression) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#115772 - ouz-a:smir_span2, r=oli-obk Improve Span in smir Addressing rust-lang/project-stable-mir#31 r? ``@oli-obk``
Addressing rust-lang/project-stable-mir#31
r? @oli-obk