Em Wed, Dec 08, 2021 at 06:34:14AM -0800, Ian Rogers escreveu:

On Wed, Dec 8, 2021 at 4:06 AM John Garry [off-list ref] wrote:

quoted

On 08/12/2021 02:45, Ian Rogers wrote:

quoted

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.

quoted

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

quoted

Thanks,
John

-- 

- Arnaldo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel