Sergei Shtylyov wrote:
> Kevin D. Kissell wrote:
>
>> I'm cool with the patch as is, but in the general spirit of regarding
>> numeric constants other than 0 and 1 as instruments of Satan, it
>> would probably be even better if those reserved bits were defined
>> (FPU_CSR_RSVD, or whatever is compatible with existing convention for
>> such bits) along with the other FCSR bit masks in mipsregs.h, so that
>> the assigment looks like:
>>
>> ctx->fcr31 = (value & ~(FPU_CSR_RSVD | 0x3)) |
>> ieee_rm[value & 0x3];
>
> 0x3 is still neither 0 nor 1, and so remains an instrument of Satan.
> How about #defining it also? :-)
In some software engineering cultures, that would be considered a good
idea or even mandatory. This being the Linux kernel, I think it's OK,
because, as Shane remarked, it's a mask that's local to the module and
whose value is "obvious", and such things are pretty commonly handled as
numeric constants in the kernel.
There actually *is* a #define for that field, FPU_CSR_RD, which could be
used here in place of the 0x3, but I'm a little torn about its use. On
one hand "value & ~(FPU_CSR_RSVD | FPU_CSR_RD)" is more clear about what
we're doing, but on the other hand, it's less obvious that "value &
FPU_CSR_RD" is generating an integer in the range 0-3 for an index. So
I'm absolutely fine with the code as written, but wouldn't complain if
someone wanted to make it use the symbolic constant.
Yours in excessive geekery,
Kevin K.
