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