Re: [PATCH 05/20] disas/nanomips: Remove __cond methods from class

[Thomas Huth <thuth@redhat.com>]

On 15/08/2022 13.07, Philippe Mathieu-Daudé wrote:
> On 15/8/22 09:26, Milica Lazarevic wrote:
>> NMD class methods with the conditional_function type like
>> NMD::ADDIU_32__cond, NMD::ADDIU_RS5__cond, etc. are removed from the NMD
>> class. They're now declared global static functions. Therefore, typedef
>> of the function pointer, conditional_function is defined outside of the
>> class.
>>
>> Now that conditional_function type functions are not part of the NMD
>> class we can't access them using the this pointer. Thus, the use of
>> the this pointer has been deleted.
>>
>> Signed-off-by: Milica Lazarevic <milica.lazarevic@syrmia.com>
>> ---
>>   disas/nanomips.cpp | 42 +++++++++++++++++++++---------------------
>>   disas/nanomips.h   | 14 ++------------
>>   2 files changed, 23 insertions(+), 33 deletions(-)
>>
>> diff --git a/disas/nanomips.cpp b/disas/nanomips.cpp
>> index 039c353d0b..9e720d0e8d 100644
>> --- a/disas/nanomips.cpp
>> +++ b/disas/nanomips.cpp
>> @@ -787,7 +787,7 @@ int NMD::Disassemble(const uint16 * data, std::string 
>> & dis,
>>               if ((op_code & table[i].mask) == table[i].value) {
>>                   /* possible match */
>>                   conditional_function cond = table[i].condition;
>> -                if ((cond == 0) || (this->*cond)(op_code)) {
>> +                if ((cond == 0) || (cond)(op_code)) {
> 
> QEMU C style is more like this:
> 
>                     if ((cond == NULL) || cond(op_code)) {
> 
>>                       try
>>                       {
>>                           if (table[i].type == pool) {
> 
>> diff --git a/disas/nanomips.h b/disas/nanomips.h
>> index a795ed44e8..0e6670adf5 100644
>> --- a/disas/nanomips.h
>> +++ b/disas/nanomips.h
>> @@ -31,6 +31,8 @@ typedef uint32_t uint32;
>>   typedef uint16_t uint16;
>>   typedef uint64_t img_address;
>> +typedef bool(*conditional_function)(uint64 instruction);
> 
> Please add a space before the returned type. I'd rather
> prefix functions extracted from the NMD class with `nmd_`:

But adding a prefix will also increase the size of the patches quite a bit 
(well, maybe not for this identifier here, but certainly for some other 
spots), so I think it's fair to keep it without prefix here, too. Just my 
0.02 €.

  Thomas