Messages in this thread | | | Date | Fri, 10 Dec 2021 16:08:12 -0300 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [PATCH 01/22] libperf: Add comments to perf_cpu_map. |
| |
Em Wed, Dec 08, 2021 at 06:34:14AM -0800, Ian Rogers escreveu: > On Wed, Dec 8, 2021 at 4:06 AM John Garry <john.garry@huawei.com> wrote: > > > > On 08/12/2021 02:45, Ian Rogers wrote: > > > diff --git a/tools/lib/perf/include/internal/cpumap.h b/tools/lib/perf/include/internal/cpumap.h > > > index 840d4032587b..1c1726f4a04e 100644 > > > --- a/tools/lib/perf/include/internal/cpumap.h > > > +++ b/tools/lib/perf/include/internal/cpumap.h > > > @@ -4,9 +4,16 @@ > > > > > > #include <linux/refcount.h> > > > > > > +/** > > > + * A sized, reference counted, sorted array of integers representing CPU > > > + * numbers. This is commonly used to capture which CPUs a PMU is associated > > > + * with. > > > + */ > > > struct perf_cpu_map { > > > refcount_t refcnt; > > > + /** Length of the map array. */ > > > int nr; > > > + /** The CPU values. */ > > > int map[]; > > > > would simply more distinct names for the variables help instead of or in > > addition to comments?
Well, in this case the typical usage doesn't help, as 'struct perf_cpu_map' are being used simply as "map" where it should be cpu_map, so we would have:
cpu_map->nr
And all should be obvious, no? Otherwise we would have redundant 'cpu', like:
cpu_map->nr_cpus
And 'map' should really be entries, so:
cpu_map->entries[index];
Would be clear enough, o?
> Thanks John! I agree. The phrase that is often used is intention > revealing names. The kernel style for naming is to be brief: > https://www.kernel.org/doc/html/v4.10/process/coding-style.html#naming > These names are both brief. nr is a little unusual, of course an > integer is a number - size and length are common names in situations > like these. In this case number makes sense as it is the number of > CPUs in the array, and there is a certain readability in saying number > of CPUs and not length or size of CPUs. The name map I have issue > with, it is always a smell if you are calling a variable a data type. > Given the convention in the context of this code I decided to leave > it. Something like array_of_cpu_values would be more intention > revealing but when run through the variable name shrinkifier could end > up as just being array, which would be little better than map. > > The guidance on comments is that they are good and to focus on the > what of what the code is doing: > https://www.kernel.org/doc/html/v4.10/process/coding-style.html#commenting > refcnt was intention revealing enough and so I didn't add a comment to it. > > > Generally developers don't always check comments where the struct is > > defined when the meaning could be judged intuitively > > Agreed. I think there could be a follow up to change to better names. > As I was lacking a better suggestion I think for the time being, and > in this patch set, we can keep things as they are. > > Thanks, > Ian > > > Thanks, > > John > >
--
- Arnaldo
| |