- 
                Notifications
    You must be signed in to change notification settings 
- Fork 13.9k
          Simplify printf and shell format suggestions
          #138125
        
          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
  
    Simplify printf and shell format suggestions
  
  #138125
              Conversation
| let sub = sub.as_str(); | ||
| if explained.contains(sub.as_ref()) { | ||
| continue; | ||
| } | ||
| explained.insert(sub); | ||
| explained.insert(String::from(sub)); | 
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.
If we're optimizing for readability here, I'd probably write this like:
if !explained.insert(sub.as_str()) {
    continue;
}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.
And possibly also change the as_str to call it to_string to make it clear that it returns a 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.
This is already on the error path, and we don't typically optimize much but the most egregious performance issues with diagnostics. Specifically, using Cow seems like a microoptimization that isn't really warranted for this code. But 🤷
0fedfb8    to
    ef6f0d6      
    Compare
  
    | It sounds like the readability approach is better, so I'll pivot to that. | 
ef6f0d6    to
    f8dc132      
    Compare
  
    printf suggestions until neededprintf and shell format suggestions
      |  | ||
| impl<'a> Substitution<'a> { | ||
| pub(crate) fn as_str(&self) -> &str { | ||
| pub(crate) fn as_string(&self) -> 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.
probably call this to_string, or maybe just implement ToString
f8dc132    to
    2458ccd      
    Compare
  
    | @bors r+ rollup | 
…gestion, r=compiler-errors Simplify `printf` and shell format suggestions Simplify tracking `printf` and shell format suggestions. Although allocations could be deferred until after checking that they aren't already in the map, this style is simpler.
…kingjubilee Rollup of 12 pull requests Successful merges: - rust-lang#136667 (Revert vita's c_char back to i8) - rust-lang#136780 (std: move stdio to `sys`) - rust-lang#137107 (Override default `Write` methods for cursor-like types) - rust-lang#137363 (compiler: factor Windows x86-32 ABI impl into its own file) - rust-lang#137528 (Windows: Fix error in `fs::rename` on Windows 1607) - rust-lang#137537 (Prevent `rmake.rs` from using unstable features, and fix 3 run-make tests that currently do) - rust-lang#137777 (Specialize `OsString::push` and `OsString as From` for UTF-8) - rust-lang#137832 (Fix crash in BufReader::peek()) - rust-lang#137904 (Improve the generic MIR in the default `PartialOrd::le` and friends) - rust-lang#138115 (Suggest typo fix for static lifetime) - rust-lang#138125 (Simplify `printf` and shell format suggestions) - rust-lang#138129 (Stabilize const_char_classify, const_sockaddr_setters) r? `@ghost` `@rustbot` modify labels: rollup
…gestion, r=compiler-errors Simplify `printf` and shell format suggestions Simplify tracking `printf` and shell format suggestions. Although allocations could be deferred until after checking that they aren't already in the map, this style is simpler.
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#136667 (Revert vita's c_char back to i8) - rust-lang#137107 (Override default `Write` methods for cursor-like types) - rust-lang#137777 (Specialize `OsString::push` and `OsString as From` for UTF-8) - rust-lang#137832 (Fix crash in BufReader::peek()) - rust-lang#137904 (Improve the generic MIR in the default `PartialOrd::le` and friends) - rust-lang#138115 (Suggest typo fix for static lifetime) - rust-lang#138125 (Simplify `printf` and shell format suggestions) - rust-lang#138129 (Stabilize const_char_classify, const_sockaddr_setters) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#138125 - thaliaarchi:defer-alloc-printf-suggestion, r=compiler-errors Simplify `printf` and shell format suggestions Simplify tracking `printf` and shell format suggestions. Although allocations could be deferred until after checking that they aren't already in the map, this style is simpler.
Simplify tracking
printfand shell format suggestions. Although allocations could be deferred until after checking that they aren't already in the map, this style is simpler.