MIME-Version: 1.0
References: <20211208024607.1784932-1-irogers@google.com> <20211208024607.1784932-2-irogers@google.com>
 <a3cf5b74-1a1b-ef85-1ad3-034e797848e2@huawei.com>
In-Reply-To: <a3cf5b74-1a1b-ef85-1ad3-034e797848e2@huawei.com>
From:   Ian Rogers <irogers@google.com>
Date:   Wed, 8 Dec 2021 06:34:14 -0800
Message-ID: <CAP-5=fV5YDghE5pHZX2+OxguZaeO_JSSXimghUGLhCaCOoCH0w@mail.gmail.com>
Subject: Re: [PATCH 01/22] libperf: Add comments to perf_cpu_map.
To:     John Garry <john.garry@huawei.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>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        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
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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?

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
>