[PATCH, V2 2/9] libsframe: refactor code for dumping section flags

[Indu Bhagat]

On 6/6/25 2:44 AM, Jan Beulich wrote:
> On 04.06.2025 09:08, Indu Bhagat via Binutils wrote:
>> @@ -40,12 +38,35 @@ is_sframe_abi_arch_aarch64 (sframe_decoder_ctx *sfd_ctx)
>>     return aarch64_p;
>>   }
>>   
>> +static void
>> +dump_sframe_header_flags (sframe_decoder_ctx *sfd_ctx)
> 
> It's really a shame that this can't be pointer-to-const. Dumping functions
> better would make quite clear that they're not altering what is being dumped.
> 
>> +{
>> +  uint8_t flags;
>> +  const char *prefix = "Flags: ";
>> +
>> +  flags = sframe_decoder_get_flags (sfd_ctx);
>> +  if (!flags)
>> +    {
>> +      printf ("%11sNONE\n", prefix);
>> +      return;
>> +    }
>> +
>> +#define PRINT_FLAG(x, y) \
>> +  if (flags & x) \
>> +    { flags = (flags & ~x); \
> 
> To play safe, please parenthesize the uses of x in expressions. Otoh ...

OK.

> 
>> +      printf ("%11s%s%s\n", prefix, y, flags ? "," : ""); \
> 
> ... this use of y doesn't need parentheses, and adding ones would only
> hamper readability.
> >> +      prefix = " "; \
>> +    }
>> +
>> +  PRINT_FLAG (SFRAME_F_FDE_SORTED, "SFRAME_F_FDE_SORTED");
>> +  PRINT_FLAG (SFRAME_F_FRAME_POINTER, "SFRAME_F_FRAME_POINTER");
> 
> This way the macro doesn't really need two parameters, does it? You
> could simply use #x there in place of y. And then actually
> 
>        printf ("%11s" #y "%s\n", prefix, flags ? "," : ""); \
> 

Ah yeah. True. Will switch to using #x.

> As to the printing of the comma: If further flags are set that the
> implementation doesn't know of, you will end up printing a comma with
> nothing following. That's odd. Perhaps print the residual at the end?
> 

My take on that is that the implementation of sframe_decode () and 
dump_sframe_header_flags () should be kept in sync when adding new 
flags.  Jens had a similar suggestion.  So now, with two folks seeing 
the value in doing so, I will add:

   /* Print any residual flags, should this implementation be out of 
sync when
      new flags are added.  */
   if (flags)
     printf ("%11s%d\n", prefix, flags);

Thanks for reviewing