Re: [PATCH 01/22] libperf: Add comments to perf_cpu_map.

From: John Garry <john.garry@huawei.com>
To: Arnaldo Carvalho de Melo <acme@kernel.org>,
	Ian Rogers <irogers@google.com>
Cc: Andi Kleen <ak@linux.intel.com>, Jiri Olsa <jolsa@redhat.com>,
	"Namhyung Kim" <namhyung@kernel.org>,
	Kajol Jain <kjain@linux.ibm.com>,
	"Paul A . Clarke" <pc@us.ibm.com>,
	Riccardo Mancini <rickyman7@gmail.com>,
	Kan Liang <kan.liang@linux.intel.com>,
	Peter Zijlstra <peterz@infradead.org>,
	"Ingo Molnar" <mingo@redhat.com>,
	Mark Rutland <mark.rutland@arm.com>,
	"Alexander Shishkin" <alexander.shishkin@linux.intel.com>,
	<linux-perf-users@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>,
	"Vineet Singh" <vineet.singh@intel.com>,
	James Clark <james.clark@arm.com>,
	"Mathieu Poirier" <mathieu.poirier@linaro.org>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Mike Leach <mike.leach@linaro.org>, Leo Yan <leo.yan@linaro.org>,
	<coresight@lists.linaro.org>,
	<linux-arm-kernel@lists.infradead.org>, <eranian@google.com>
Subject: Re: [PATCH 01/22] libperf: Add comments to perf_cpu_map.
Date: Mon, 13 Dec 2021 08:56:17 +0000	[thread overview]
Message-ID: <02b330aa-ebce-11b2-9d3f-9b2497ae7133@huawei.com> (raw)
In-Reply-To: <YbOlnH0NLQdAECcK@kernel.org>

On 10/12/2021 19:08, Arnaldo Carvalho de Melo wrote:
>>>> +/**
>>>> + * 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;

I'd have /s/nr/len/, as it means the map length, as opposed to confusing 
nr meaning with number of cpus in the host or something else. And the 
new comment uses "Length" also.

>>>> +     /** 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"

There are a lot of instances to change ... but I am all up for using 
consistent and well-meaning variable / argument names per type.

> 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:

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