issue #190: added frequency per core on linux#1593
issue #190: added frequency per core on linux#1593FlavioMili wants to merge 5 commits intoaristocratos:mainfrom
Conversation
| export PATH="/usr/pkg/sbin:/usr/pkg/bin:$PATH" | ||
| export PKG_PATH="https://ftp.netbsd.org/pub/pkgsrc/packages/NetBSD/${{ matrix.arch }}/${{ matrix.version }}/All/" | ||
| /usr/sbin/pkg_add pkgin | ||
| pkgin -y install cmake gcc14 git ninja-build |
There was a problem hiding this comment.
I honestly thought so but after adding that and the retry, it stopped failing. Let me see better what I can do
There was a problem hiding this comment.
What I wanted to say is, even if this is needed to fix something in the CI, it should be handled separately from this PR.
But, this is just IMHO, I'll let the maintainer decide if he's OK with it here. Don't be distracted by my nitpicking. ;-)
Just a random passer-by...
| if (Runner::stopping) return ""; | ||
| if (force_redraw) redraw = true; | ||
| bool show_temps = (Config::getB("check_temp") and got_sensors); | ||
| #ifdef __linux__ |
There was a problem hiding this comment.
Just thinking out loud here...
Cannot Config::getB(...) be made to return false by default, and then enabling it (true) only for platforms where it is working ?
That would alleviate the need for those #ifdefs...
There was a problem hiding this comment.
I had written it like this because I had seen (just a couple lines up) that #ifdefs are used around the code so I thought it would be ok.
I think it would be easier to do if the frequency per core was not included in the show_cpu_freq but at the same time it would make sense to be set only if we are actually showing the cpu freqs.
In that case I would need to add settings and also some additional settings that appear only in special case.
I think at this point it would be easier to just make sure that everything works and compiles on any platform, thing that at the moment I have no opportunity to try. Do you have any suggestion on how to try this?
This is the exact type of code we do not want. You should always be doing the due diligence of verifying your AI assisted code as if you had written it yourself. |
|
Who even said I didn't read and review the code? |
3 participants
The PR is AI assisted, for the UI I didn't want to spend too much time on calculating width and different things, I just found it working right away this way and I didn't need to learn the API of fmt :D.
I decided to display only up to 4 characters by representing either xxxM or x.xG for MHz and GHz values, it should be enough and saves 2 characters to keep the core consumption bar long enough.
Hopefully someone can also add the frequency for the other platforms, I don't know how to test those.
Please tell me if you need me to edit or improve something, thank you.