[DTrace-devel] [PATCH 03/14] Promote associative integer arrays to 64 bits when loaded

[Eugene Loh]

On 5/3/23 23:54, Kris Van Hees wrote:

> On Mon, May 01, 2023 at 11:47:11PM -0400, eugene.loh--- via DTrace-devel wrote:
>> From: Eugene Loh <eugene.loh at oracle.com>
>>
>> In commit 7e9ce1eee475 ("Promote integers to 64 bits when loaded"),
>> integers were promoted to 64 bits when loaded.  Associative arrays
>> were on a different code path.
>>
>> Add the promotion to the associative-array code path and add tests
>> for different variable types.  Also, do some minor refactoring in
>> dt_cg_assoc_op() in anticipation of subsequent refactoring; this
>> prep work is done here to improve readability of subsequent patches.
> OK on the addition of the promotion.
>
>> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
>> @@ -3988,7 +3988,7 @@ dt_cg_asgn_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>>   }
>>   
>>   static void
>> -dt_cg_assoc_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>> +dt_cg_assoc_op(dt_node_t *dst, dt_irlist_t *dlp, dt_regset_t *drp)
> I do not really agree with this change (and the resulting changes below)...
> This is a function that generates code for a node, and the convention in the
> code generator is that dnp is used for that variable.  The change to 'dst'
> seems to imply that this node is the destination of some operation, but then
> one would expect there to be a source as well, and that is clearly not the
> case.
>
> Let's keep this as-is, please.

I hear you, but this change isn't meant to stand on its own.  It's setting up some code refactoring... in essence, the code being modified here is going to disappear in later patches.  The changes in this patch are supposed to be easy to review ("Oh yeah, I can see he's simply doing a simple renaming") and trickier stuff will be in later patches.

Ultimately, the point is to combine tvar and assoc access in a way that common code can be extracted to simplify the code and make it easier to make changes (e.g., to support NULL strings).

So I get your point on this patch in isolation, but I do not understand what you're suggesting for the overall roadmap.  I would think combining tvar and assoc more is unequivocally a good thing.