On 2/1/2023 11:45 AM, Dave Hansen wrote:
> On 1/31/23 15:43, Jithu Joseph wrote:
>> +static void ifs_array_test_core(int cpu, struct device *dev)
>> +{
>> + union ifs_array activate, status;
>> + bool timed_out = false;
>> + struct ifs_data *ifsd;
>> + unsigned long timeout;
>> + u64 msrvals[2];
>> +
>> + ifsd = ifs_get_data(dev);
>> +
>> + activate.data = 0;
>> + activate.array_bitmask = ~0U;
>> + activate.ctrl_result = 0;
>
> I think this whole 'ifs_array' as a union thing is bogus. It's actually
> obfuscating and *COMPLICATING* the code more than anything. Look what
> you have:
>
> union ifs_array activate; // declare it on the stack, unzeroed
>
> activate.data = 0; // zero the structure;
> activate.array_bitmask = ~0U; // set one field
> activate.ctrl_result = 0; // set the field to zero again???
>
> Can we make it less obfuscated? How about:
>
> struct ifs_array activate = {}; // zero it
> ...
> activate.array_bitmask = ~0U; // set the only nonzero field
>
> Voila! Less code, less obfuscation, less duplicated effort. Or, worst
Agreed, will modify it as you suggest above to remove the duplicate zero assignments
> case:
>
> struct ifs_array activate;
> ...
> memset(&activate, 0, sizeof(activate));
> activate.array_bitmask = ~0U;
>
> That's sane and everyone knows what it does and doesn't have to know
> what unions are involved or how they are used. It's correct code no
> matter *WHAT* craziness lies within 'activate'.
