Skip to content

Hide argument name hint regardless of case#7215

Merged
bors[bot] merged 1 commit intorust-lang:masterfrom
jhpratt:master
Jan 9, 2021
Merged

Hide argument name hint regardless of case#7215
bors[bot] merged 1 commit intorust-lang:masterfrom
jhpratt:master

Conversation

@jhpratt
Copy link
Member

@jhpratt jhpratt commented Jan 9, 2021

No description provided.

@SomeoneToIgnore
Copy link
Contributor

One small nit: could you try and use make_ascii_lowercase instead of its to_ counterpart?
The recent trend is to avoid unnecessary allocations and this case sounds like a place worth to try: we run this filter_map for every parameter hint in the whole document open, which can be big.

@jhpratt
Copy link
Member Author

jhpratt commented Jan 9, 2021

For the parameter name at least, it's not currently taken as mutable. By using make_ascii_lowercase, it would affect every usage of the variable. I can do this if desired, but I presume it would cascade and may have unintended consequences.

@SomeoneToIgnore
Copy link
Contributor

Yes, should have elaborated a bit more: don't bother with parameter_name, it's &str anyway, but the repr could be processed as

Some(mut repr) => {
    let param_name = param_name.to_ascii_lowercase();
    let argument_string = {
        repr.make_ascii_lowercase();
        repr.trim_start_matches('_')
    };
    argument_string.starts_with(&param_name) || argument_string.ends_with(&param_name)
}

or simiar way, but the point is that it's a String so no need to reallocate it more.

@jhpratt
Copy link
Member Author

jhpratt commented Jan 9, 2021

Ah, gotcha. Of course I was just playing around wondering why it wasn't working, and didn't even think to reorder the make_ascii_lowercase and trim_start_matches. I wasn't sure the extent you meant because you seemed to have wanted me to break other things 😛

Regardless, I've just pushed up an updated commit without compile errors and including a trivial test case.

@SomeoneToIgnore
Copy link
Contributor

SomeoneToIgnore commented Jan 9, 2021

you seemed to have wanted me to break other things

That would be unfair ot lure you into a small PR and ask for big breakings in other places, so no.
Thanks again!

bors r+

@jhpratt
Copy link
Member Author

jhpratt commented Jan 9, 2021

Admittedly I wouldn't have minded, just wanted to be clear on what you wanted haha

Thanks for the quick feedback and guidance!

@bors
Copy link
Contributor

bors bot commented Jan 9, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants