Skip to content

Three small fixes for save-analysis #43533

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Aug 1, 2017
Merged

Three small fixes for save-analysis #43533

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
66702b4
save-analysis: dedup macro references
nrc Jul 25, 2017
e1206c4
save-anlaysis: fix filter_generated
nrc Jul 28, 2017
bbc00c9
format!: use a dummy span rather than callee span for the span base f…
nrc Jul 28, 2017
27b9182
review changes
nrc Aug 1, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/librustc_save_analysis/dump_visitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ use rustc::hir::map::Node;
use rustc::session::Session;
use rustc::ty::{self, TyCtxt};

use std::collections::HashSet;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we be using FxHashSet in the compiler?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, although I image the performance impact would be negligible here.

use std::path::Path;

use syntax::ast::{self, NodeId, PatKind, Attribute, CRATE_NODE_ID};
Expand Down Expand Up @@ -74,6 +75,7 @@ pub struct DumpVisitor<'l, 'tcx: 'l, 'll, O: DumpOutput + 'll> {
// we only write one macro def per unique macro definition, and
// one macro use per unique callsite span.
// mac_defs: HashSet<Span>,
macro_calls: HashSet<Span>,
}

impl<'l, 'tcx: 'l, 'll, O: DumpOutput + 'll> DumpVisitor<'l, 'tcx, 'll, O> {
Expand All @@ -89,6 +91,7 @@ impl<'l, 'tcx: 'l, 'll, O: DumpOutput + 'll> DumpVisitor<'l, 'tcx, 'll, O> {
span: span_utils.clone(),
cur_scope: CRATE_NODE_ID,
// mac_defs: HashSet::new(),
macro_calls: HashSet::new(),
}
}

Expand Down Expand Up @@ -972,11 +975,19 @@ impl<'l, 'tcx: 'l, 'll, O: DumpOutput + 'll> DumpVisitor<'l, 'tcx, 'll, O> {
/// callsite spans to record macro definition and use data, using the
/// mac_uses and mac_defs sets to prevent multiples.
fn process_macro_use(&mut self, span: Span) {
let source_span = span.source_callsite();
if self.macro_calls.contains(&source_span) {
return;
}
self.macro_calls.insert(source_span);

let data = match self.save_ctxt.get_macro_use_data(span) {
None => return,
Some(data) => data,
};

self.dumper.macro_use(data);

// FIXME write the macro def
// let mut hasher = DefaultHasher::new();
// data.callee_span.hash(&mut hasher);
Expand All @@ -996,7 +1007,6 @@ impl<'l, 'tcx: 'l, 'll, O: DumpOutput + 'll> DumpVisitor<'l, 'tcx, 'll, O> {
// }.lower(self.tcx));
// }
// }
self.dumper.macro_use(data);
}

fn process_trait_item(&mut self, trait_item: &'l ast::TraitItem, trait_id: DefId) {
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_save_analysis/span_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ impl<'a> SpanUtils<'a> {

// Otherwise, a generated span is deemed invalid if it is not a sub-span of the root
// callsite. This filters out macro internal variables and most malformed spans.
!parent.source_callsite().contains(parent)
!parent.source_callsite().contains(sub_span.unwrap())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm weary of unwrap(). Even though it the code is valid now, somebody could come later and inadvertently remove the lines 401-403 without changing this. Could you change the above lines to if let Some(sub_span), else return true;? I know it's a nitpick.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or just change 401-403 to let sub_span = unwrap_or!(sub_span, return true); (unwrap_or!).

}
}

Expand Down
8 changes: 5 additions & 3 deletions src/libsyntax_ext/format.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use syntax::ext::build::AstBuilder;
use syntax::parse::token;
use syntax::ptr::P;
use syntax::symbol::{Symbol, keywords};
use syntax_pos::Span;
use syntax_pos::{Span, DUMMY_SP};
use syntax::tokenstream;

use std::collections::{HashMap, HashSet};
Expand Down Expand Up @@ -558,8 +558,10 @@ impl<'a, 'b> Context<'a, 'b> {
// passed to this function.
for (i, e) in self.args.into_iter().enumerate() {
let name = self.ecx.ident_of(&format!("__arg{}", i));
let span =
Span { ctxt: e.span.ctxt.apply_mark(self.ecx.current_expansion.mark), ..e.span };
let span = Span {
ctxt: e.span.ctxt.apply_mark(self.ecx.current_expansion.mark),
..DUMMY_SP
};
pats.push(self.ecx.pat_ident(span, name));
for ref arg_ty in self.arg_unique_types[i].iter() {
locals.push(Context::format_arg(self.ecx, self.macsp, e.span, arg_ty, name));
Expand Down