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

Fix logic bug in cache_nvidia_string_value_update() #2022

Merged
merged 3 commits into from
Aug 29, 2024

Conversation

w0j0pl
Copy link
Contributor

@w0j0pl w0j0pl commented Aug 28, 2024

Checklist

  • I have described the changes
  • I have linked to any relevant GitHub issues, if applicable
  • Documentation in doc/ has been updated
  • All new code is licensed under GPLv3

Description

  • Describe the changes, why they were necessary, etc
  • Describe how the changes will affect existing behaviour.
  • Describe how you tested and validated your changes.
  • Include any relevant screenshots/evidence demonstrating that the changes work and have been tested.

Made a suggested change in logic from issue brndnmtthws#1177
Made a suggested change in logic from issue brndnmtthws#1178
Made a suggested change in logic from issue brndnmtthws#1178
Copy link

netlify bot commented Aug 28, 2024

Deploy Preview for conkyweb canceled.

Name Link
🔨 Latest commit 5851be9
🔍 Latest deploy log https://app.netlify.com/sites/conkyweb/deploys/66cf4dc8dee60c00084cb5f0

@github-actions github-actions bot added sources PR modifies project sources nvidia Issue or PR related to nvidia cards labels Aug 28, 2024
@brndnmtthws
Copy link
Owner

Are we certain this is the correct fix?

@w0j0pl
Copy link
Contributor Author

w0j0pl commented Aug 29, 2024

I read what the z1atk0 guy wrote, checked this and for me it doesn't make any sense to check if memTransferRatemax is cached or not if we want to cache perfmin or perfmax. It makes sense to check if perfmin is cached if we want to know wether or not we should cache it. And the same story with perfmax.

Also if you check all of the else ifs right above this one, you will see that the we check if something is cached, before we cache this thing. For me it looks like copy/paste, but someone forgot to change memTransferRatemax in the second place.

I won't give you 100% certainty, because I'm just a noobie and it's my 2nd change in such a big project, but for me everything looks fine.

@brndnmtthws brndnmtthws changed the title Made a suggested change in logic from issue #1178 Fix logic bug in cache_nvidia_string_value_update() Aug 29, 2024
@brndnmtthws brndnmtthws merged commit 03711ea into brndnmtthws:main Aug 29, 2024
39 checks passed
@brndnmtthws brndnmtthws added the bug Bug report or bug fix PR label Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug report or bug fix PR nvidia Issue or PR related to nvidia cards sources PR modifies project sources
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants