Skip to content
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

Remove always-true condition #3009

Merged
merged 1 commit into from
Jan 7, 2021
Merged

Remove always-true condition #3009

merged 1 commit into from
Jan 7, 2021

Conversation

Youssef1313
Copy link
Member

No description provided.

@Youssef1313 Youssef1313 requested a review from a team as a code owner May 17, 2020 11:47
@ghost ghost added the PR metadata: Label to tag PRs, to facilitate with triage label May 17, 2020
@ghost ghost requested review from fabiant3, ryalanms and SamBent May 17, 2020 11:47
@@ -142,7 +142,7 @@ public override object ConvertTo(ITypeDescriptorContext context, CultureInfo cul

private object GetKey(string keyToken, CultureInfo culture)
{
if (keyToken == String.Empty)
if (keyToken.Length == 0)
Copy link
Member

Choose a reason for hiding this comment

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

NullReferenceException?

Could you use string.IsNullOrEmpty ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The caller code is:

                string fullName = ((string)source).Trim();
                object key = GetKey(fullName, CultureInfo.InvariantCulture);

Trim won't return null, so fullName is going to be a non-null value.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you looked at the old code and traced it if the passed keyToken is null, You'll find that it will throw NullReferenceException at:

                keyToken = keyToken.ToUpper(culture);

But, anyway the passed value isn't null. So, it's okay.

@lindexi
Copy link
Member

lindexi commented May 18, 2020

Thank you @Youssef1313

And I think the if (keyToken.Length == 0) code doesn't match the subject. Your subject is Remove always-true condition but this code is to improve performance.

@Youssef1313
Copy link
Member Author

Your subject is Remove always-true condition but this code is to improve performance.

That's correct. I just did both because they're in the same file and the change is really small.

@ryalanms
Copy link
Member

ryalanms commented Jan 7, 2021

Thanks, @Youssef1313.

@ryalanms ryalanms merged commit 53eafe1 into dotnet:master Jan 7, 2021
@Youssef1313 Youssef1313 deleted the patch-3 branch January 7, 2021 13:25
@ghost ghost locked as resolved and limited conversation to collaborators Apr 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants