[DTrace-devel] [PATCH 07/38] Clean up prp/pprp/uprp variable names

[Eugene Loh]

On 7/18/24 14:48, Kris Van Hees wrote:

> On Thu, Jun 27, 2024 at 01:34:24AM -0400, eugene.loh at oracle.com wrote:
>> diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
>> @@ -6,6 +6,31 @@
>> +/*
>> + * This file uses both overlying probes (specified by the user) as well as
>> + * underlying probes (the uprobes recognized by the kernel).  To minimize
>> + * confusion, this file should use consistent variable names:
>> + *
>> + *     dt_probe_t	*prp;   //  overlying probe
>> + *     dt_probe_t	*uprp;  // underlying probe
>> + *
>> + *             Either one might be returned by dt_probe_lookup() or
>> + *             dt_probe_insert() or added to dt_enablings[] or dt_probes[].
>> + *             Further, uprp might be returned by create_underlying().
>> + *
>> + *     dt_uprobe_t	*upp;   // uprobe associated with an underlying probe
>> + *
>> + *     list_probe_t	*pop;   //  overlying probe list
>> + *     list_probe_t	*pup;   // underlying probe list
>> + *
>> + * The provider-specific prv_data has these meanings:
>> + *
>> + *     prp->prv_data            // dt_list_t of associated underlying probes
>> + *
>> + *     uprp->prv_data           // upp (the associated uprobe)
>> + *
>> + * Finally, note that upp->probes is a dt_list_t of overlying probes.
>> + */
> As mentioned in my earlier review, I truly believe that this comment block is
> not necessary because the code changes includes in the patch accomplish what
> is described here and I think that sets up enough of a precedent/example that
> future changes in code should stick to that naming pattern.  No need for an
> explicit comment block to highlight that.

One more round of negotiation, but then I'll go with whatever you 
respond in your next reply.

I'm a fan of code that is so well written that there is no need for 
comments.

But at the same time, if we rely on maintainers to read between the 
lines what is intended, there is a risk of things going wrong. E.g., 
someone might come from another file where they're used to prp for every 
probe (which is what was going on in this file, making things somewhat 
confusing in my mind).  Also, the comment points out the important 
distinction that prv_data means different things for different probes.

The comment block seems to me to be a clear statement of what practice 
to employ in this file.  I've spent a bunch of time reverse-engineering 
code to figure out what was going on in some writer's mind.  It becomes 
clear once I figure it out, but an explanation would have been so welcome.

Again, though, I'll go with whichever way you say in your next reply.