Three small fixes for save-analysis#43533
Conversation
estebank
left a comment
estebank
left a comment
There was a problem hiding this comment.
Small nitpicks, looks good to merge otherwise.
| // 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()) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Or just change 401-403 to let sub_span = unwrap_or!(sub_span, return true); (unwrap_or!).
| use rustc::session::Session; | ||
| use rustc::ty::{self, TyCtxt}; | ||
|
|
||
| use std::collections::HashSet; |
There was a problem hiding this comment.
Shouldn't we be using FxHashSet in the compiler?
There was a problem hiding this comment.
Probably, although I image the performance impact would be negligible here.
|
Reviewed, r=me modulo existing discussion. |
carols10cents
added
the
S-waiting-on-author
|
Thanks for the reviews, updated with changes. @bors: r=jseyfried, estebank |
|
📌 Commit 27b9182 has been approved by |
nrc
added
S-waiting-on-bors
Three small fixes for save-analysis First commit does some naive deduplication of macro uses. We end up with lots of duplication here because of the weird way we get this data (we extract a use for every span generated by a macro use). Second commit is basically a typo fix. Third commit is a bit interesting, it partially reverts a change from #40939 where temporary variables in format! (and thus println!) got a span with the primary pointing at the value stored into the temporary (e.g., `x` in `println!("...", x)`). If `format!` had a definition it should point at the temporary in the macro def, but since it is built-in, that is not possible (for now), so `DUMMY_SP` is the best we can do (using the span in the callee really breaks save-analysis because it thinks `x` is a definition as well as a reference). There aren't a test for this stuff because: the deduplication is filtered by any of the users of save-analysis, so it is purely an efficiency change. I couldn't actually find an example for the second commit that we have any machinery to test, and the third commit is tested by the RLS, so there will be a test once I update the RLS version and and uncomment the previously failing tests). r? @jseyfried
|
☀️ Test successful - status-appveyor, status-travis |
5 participants
First commit does some naive deduplication of macro uses. We end up with lots of duplication here because of the weird way we get this data (we extract a use for every span generated by a macro use).
Second commit is basically a typo fix.
Third commit is a bit interesting, it partially reverts a change from #40939 where temporary variables in format! (and thus println!) got a span with the primary pointing at the value stored into the temporary (e.g.,
xinprintln!("...", x)). Ifformat!had a definition it should point at the temporary in the macro def, but since it is built-in, that is not possible (for now), soDUMMY_SPis the best we can do (using the span in the callee really breaks save-analysis because it thinksxis a definition as well as a reference).There aren't a test for this stuff because: the deduplication is filtered by any of the users of save-analysis, so it is purely an efficiency change. I couldn't actually find an example for the second commit that we have any machinery to test, and the third commit is tested by the RLS, so there will be a test once I update the RLS version and and uncomment the previously failing tests).
r? @jseyfried