clbr / radeontop

GNU General Public License v3.0
804 stars 70 forks source link

Bobs fixes #22

Closed bobsayshilol closed 9 years ago

bobsayshilol commented 9 years ago

Here's a collection of changes I made after I repeatedly kept forgetting to add -c, and then decided to run it through valgrind - sorry they're not split up!

I also have another local change that seems to make valgrind happier (~400KB -> ~20KB lost), but I don't think it's worth committing so here's the diff instead:

diff --git a/ui.c b/ui.c
index 3a9fa75..04e729c 100644
--- a/ui.c
+++ b/ui.c
@@ -84,7 +84,9 @@ void present(const unsigned int ticks, const char card[], unsigned int color) {
        while(!results)
                usleep(16000);

-       initscr();
+       SCREEN *screen = newterm(NULL, stdout, stdin);
+       set_term(screen);
+
        noecho();
        halfdelay(10);
        curs_set(0);
@@ -236,4 +238,5 @@ void present(const unsigned int ticks, const char card[], unsigned int color) {
        }

        endwin();
+       delscreen(screen);
 }
clbr commented 9 years ago

The logic fix commit is wrong, there is no major version 3. It is not the kernel's version, it's the radeon drm version; today's git says 2.42. Major version 3 would mean big enough changes that existing radeontop would probably not work. Please drop that commit.

Releasing pciaccess resources is ok, but should be done in init_pci; committed. Releasing ncurses resources that exist for the program's lifetime is unnecessary.

The pedantic commit is good, one style quip about the cast: it should have a space before * and after ). The color change is also ok, but please update the documentation (man page source, the .asc file).

bobsayshilol commented 9 years ago

Looking back at this I should not commit or post pull requests when tired.

The logic fix commit is wrong

Ah I thought this was a typo that had gone unnoticed. Still, I think something can be done for readability there to make sure others don't slip up as I did - maybe ver->version_major != 2 instead of < 2?

Releasing ncurses resources that exist for the program's lifetime is unnecessary.

I'd agree. I blame tiredness.

one style quip about the cast: it should have a space before * and after )

Don't get me started on the misspelling of "colour" :P

please update the documentation

Will do.

I'll cherry-pick this pull request into 2 (1 for toggleable colour support, and 1 for me being too pedantic) and re-submit them.