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

Handle Ryzen CPU and AMD GPU vendor string and break on first regex match #250

Merged
merged 2 commits into from
Oct 1, 2022

Conversation

vjr
Copy link
Member

@vjr vjr commented Sep 28, 2022

Some fixups to handle specific Ryzen CPU and AMD GPU vendor strings in the regex replacement cleanup method.

Also, break after the first regex match, so the replacement strings are in order of preference now.

Before: (note the lowercasing of the cpu string and lengthy gpu string)

about_before

After: (note the generic gpu string part matched, instead of the "RX 570" trailer because the actual gpu is an RX 580)

about_after

@vjr vjr requested a review from a team September 28, 2022 09:54
@vjr
Copy link
Member Author

vjr commented Sep 28, 2022

Do we want to have "AMD®" (the registered trademark symbol) prefixed on the GPU (and CPU?) vendor string displayed?

lenemter
lenemter previously approved these changes Sep 28, 2022
Copy link
Member

@lenemter lenemter left a comment

Choose a reason for hiding this comment

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

Works for me

Before:
image
After:
image

@vjr
Copy link
Member Author

vjr commented Sep 28, 2022

@lenemter I update the PR with an addition regex - could you see if it cleans up your iGPU vendor string? (post updated screenshot)?

@vjr vjr requested review from a team and lenemter September 28, 2022 10:30
Copy link
Member

@lenemter lenemter left a comment

Choose a reason for hiding this comment

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

Thanks! Looks a lot better now

image

@lenemter
Copy link
Member

® is too small in this font, should we keep it?

@vjr
Copy link
Member Author

vjr commented Sep 28, 2022

Thanks @lenemter ! I will let others (core devs) comment/merge as they see fit :-)

@vjr
Copy link
Member Author

vjr commented Sep 28, 2022

® is too small in this font, should we keep it?

Seems OK to me (there is in existing code as well) but I don't have opinion either way.

@vjr vjr requested a review from a team September 30, 2022 09:02
Copy link
Member

@ryonakano ryonakano left a comment

Choose a reason for hiding this comment

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

Code looks good and works as expected for me.

Before

スクリーンショット 2022-10-01 09 11 52

After

スクリーンショット 2022-10-01 09 12 35

@vjr vjr merged commit bd38ffc into master Oct 1, 2022
@vjr vjr deleted the ryzen-cpu-name branch October 1, 2022 16:02
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.

3 participants