Hi,
On Wed, Apr 11, 2018 at 02:02:02PM +0300, Artem Bityutskiy wrote:
> A couple of nitpicks.
>
> On Wed, 2018-04-11 at 18:30 +0800, Yu Chen wrote:
> > @@ -48,6 +48,7 @@ char *proc_stat = "/proc/stat";
> > FILE *outf;
> > int *fd_percpu;
> > struct timespec interval_ts = {5, 0};
> > +int iterations;
>
> OK, out of several choices, you selected "iterations".
>
> > unsigned int debug;
> > unsigned int quiet;
> > unsigned int sums_need_wide_columns;
> > @@ -470,6 +471,7 @@ void help(void)
> > " {core | package | j,k,l..m,n-p }\n"
> > "--quiet skip decoding system configuration header\n"
> > "--interval sec Override default 5-second measurement interval\n"
> > + "--iterations loops The number of loops if interval is specified\n"
>
> Since "iterations" is the term, be consistent and do not mix it with
> "loops". Who knows may be the "loops" term will be used for something
> else in the future. Use something like this:
>
> "--iterations count Number of measurement iterations (requires '
> --interval')"
>
OK, this looks more consistent.
> > print this help mkk
> > "--list list column headers only\n"
> > "--out file create or truncate \"file\" for all output\n"
> > @@ -2565,6 +2567,7 @@ void turbostat_loop()
> > {
> > int retval;
> > int restarted = 0;
> > + int loops = 0;
>
> Please, name variables in a consistent manner, this should really be
> something like 'int iters = 0'. Or may be 'done_iters', or something.
> But not "loops".
>
OK.
> > @@ -4999,6 +5010,7 @@ void cmdline(int argc, char **argv)
> > {"Dump", no_argument, 0, 'D'},
> > {"debug", no_argument, 0, 'd'}, /*
> > internal, not documented */
> > {"interval", required_argument, 0, 'i'},
> > + {"iterations", required_argument, 0, 't'},
>
> If you used term "count", you could have consistent long and short
> option names, like '--count / -c'. I find '--iterations / -t' to be
> inconsistent, and harder to remember the short option, because I think
> about time, not "iterations" when I see -t.
However the '-c' is already used as a short form for '--cpu',
so I chose --iterations previously.
Thanks,
Yu
