-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fix not disable string escape highlights #21420
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
Conversation
| let mut hl = highlights::Highlights::new(root.text_range()); | ||
| let krate = sema.scope(&root).map(|it| it.krate()); | ||
| traverse(&mut hl, &sema, config, InRealFile::new(file_id, &root), krate, range_to_highlight); | ||
| hl.retain_mut(|hl_range| filter_by_config(&mut hl_range.highlight, config)); |
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 it better to filter the entire tree here, or is it better to pass config to each add?
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.
I believe it's better to filter on the spot, not the entire tree. This is faster, and the tree can be large.
| self.root.add(hl_range); | ||
| } | ||
|
|
||
| pub(crate) fn retain_mut(&mut self, mut f: impl FnMut(&mut HlRange) -> bool) { |
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.
Can call this just retain(), the reason Vec::retain_mut() is named this way is due to a past mistake and it's IMO not a good name. Ditto for below.
| let mut hl = highlights::Highlights::new(root.text_range()); | ||
| let krate = sema.scope(&root).map(|it| it.krate()); | ||
| traverse(&mut hl, &sema, config, InRealFile::new(file_id, &root), krate, range_to_highlight); | ||
| hl.retain_mut(|hl_range| filter_by_config(&mut hl_range.highlight, config)); |
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.
I believe it's better to filter on the spot, not the entire tree. This is faster, and the tree can be large.
Example
---
with config `strings: false`
```rust
fn main() {
format_args!("foo\nbar\invalid");
}
```
**Before this PR**
```rust
fn main() {
format_args!("foo\nbar\invalid");
// ^^ EscapeSequence
// ^^ InvalidEscapeSequence
}
```
**After this PR**
```rust
fn main() {
format_args!("foo\nbar\invalid");
}
```
50de646 to
fa3d8af
Compare
ChayimFriedman2
left a comment
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.
Thanks!
Example
with config
strings: falseBefore this PR
After this PR