Re: [PATCH 1/3] [ARC] Add support for address type syntax

[Graham Markall <graham dot markall at embecosm dot com>]

Hi Claudiu,

Many thanks for the review - I have posted a new series of patches in
which I have tried to address your comments on all three patches:
https://sourceware.org/ml/binutils/2016-07/msg00324.html

Best regards,
Graham.

On 22/07/16 17:52, Claudiu Zissulescu wrote:
> Hi,
>>
>> +/* Used to define a colon as an operand in tokens */
> 
> Dot space space.
> 
>> +#define O_colon O_md31
>> +
>> +/* Used to define address types in nps400 */
> 
> Same here.
> 
>> +#define O_addrtype O_md30
>> +
>>   /* Dummy relocation, to be sorted out.  */
>>   #define DUMMY_RELOC_ARC_ENTRY     (BFD_RELOC_UNUSED + 1)
>>
>> @@ -979,6 +988,8 @@ debug_exp (expressionS *t)
>>       case O_logical_or:        name = "O_logical_or";        break;
>>       case O_index:        name = "O_index";        break;
>>       case O_bracket:        name = "O_bracket";        break;
>> +    case O_colon:               name = "O_colon";               break;
>> +    case O_addrtype:            name = "O_addrtype";            break;
> 
> Groups of 8 spaces needs to be replaced by tabs.
>>         if ((operand->flags & ARC_OPERAND_FAKE)
>> -          && !(operand->flags & ARC_OPERAND_BRAKET))
>> +          && !((operand->flags & ARC_OPERAND_BRAKET) ||
>> +                   (operand->flags & ARC_OPERAND_COLON)))
> 
> May make sense to craft a macro here, as this construction is used
> multiple time.
> 
>> +            case ARC_OPERAND_ADDRTYPE:
>> +              /* Check to be an address type */
> 
> Dot space space.
> 
>> +            case ARC_OPERAND_COLON:
>> +              /* Check if colon is also in opcode table as operand */
> 
> Same here.
> 
>> -      && !(operand->flags & ARC_OPERAND_BRAKET))
>> +      && !((operand->flags & ARC_OPERAND_BRAKET) ||
>> +               (operand->flags & ARC_OPERAND_COLON)))
> 
> Here you can reuse the macro above suggested.
> 
>> +/* Address type operand for NPS400  */
> 
> Dot space space.
> 
>> +#define ARC_OPERAND_ADDRTYPE    0x2000
>> +
>> +/* Mark the colon position     */
> 
> Same here.
> 
>> +  /* CMEM Extended Summarized Address.  */
>> +  ARC_NPS400_ADDRTYPE_CXD,
> 
> This last comma may trigger warnings with old compilers.
> 
>>
>> +static const char *
> 
> Missing a short description of the function.
> 
>> +get_addrtype (int value)
>> +{
>> +  if (value < 0 || value > addrtypenames_max)
>> +    return addrtypeunknown;
>> +
>> +  return addrtypenames[value];
>> +}
>> -      && !(operand->flags & ARC_OPERAND_BRAKET))
>> +      && !((operand->flags & ARC_OPERAND_BRAKET) ||
>> +               (operand->flags & ARC_OPERAND_COLON)))
> 
> Macro used here.
> 
> Thanks,
> Claudiu

Attachment: signature.asc
Description: OpenPGP digital signature