Hi Nick,

On 1/5/24 10:57, Nick Clifton wrote:
> Hi Srinath,
> 
>> This patch will not be committed to development branch until the 
>> Kernel GCS
>> ABI changes are approved and committed.
>>
>> Ok to commit this patch to Arm vendor branch based on binutils-master ?
> 
> Yes.  (Since the branch is ARM specific, you do not really need to check
> with us first, but the thought is appreciated).

Thanks Nick, I made below proposed changes in v2 patch, created 
"users/ARM/gcs-binutils-gdb-master" branch and committed the patch 
(attached) to this branch.

Regards,
Srinath.

> Looking over the patch I did see one thing which niggled:
> 
> +     else if (strncmp (optarg, "experimental-gcs-report", 23) == 0)
> +    {
> +      if (strlen (optarg) == 23 || strcmp (optarg + 23, "=none") == 0)
> +        gcs_report = GCS_NONE;
> +      else if (strcmp (optarg + 23, "=warning") == 0)
> +        gcs_report = GCS_WARN;
> +      else if (strcmp (optarg + 23, "=error") == 0)
> +        gcs_report = GCS_ERROR;
> +      else
> +        einfo (_("%P: error: unrecognized: `%s'\''\n"), optarg);
> +    }
> +     else if (strncmp (optarg, "experimental-gcs", 16) == 0)
> +    {
> +      if (strlen (optarg) == 16 || strcmp (optarg + 16, "=always") == 0)
> +        gcs_type = GCS_ALWAYS;
> +      else if (strcmp (optarg + 16, "=never") == 0)
> +        gcs_type = GCS_NEVER;
> +      else if (strcmp (optarg + 16, "=implicit") == 0)
> +        gcs_type = GCS_IMPLICIT;
> +      else
> +        einfo (_("%P: error: unrecognized: `%s'\''\n"), optarg);
> +    }
> 
> I do not like the use of magic numbers (23 and 16 in the fragment above).
> Whilst it is fairly obvious where they come from, it is entirely possible
> that a future coder might change one of the strings and not change (all
> of) the places where the string's length is used.  So instead I would
> suggest using constants instead, eg:
> 
>        static const char * egr = "experimental-gcs-report";
>        const int egr_len = strlen (egr);
> 
>        else if (strncmp (optarg, egr, egr_len) == 0)
>      {
>        if (strlen (optarg) == egr_len || strcmp (optarg + egr_len, 
> "=none") == 0)
>          gcs_report = GCS_NONE;
>        else if (strcmp (optarg + egr_len, "=warning") == 0)
>          gcs_report = GCS_WARN;
>        else if (strcmp (optarg + egr_len, "=error") == 0)
>          gcs_report = GCS_ERROR;
>        else
>          einfo (_("%P: error: unrecognized: `%s'\''\n"), optarg);
>      }
> 
> Cheers
>    Nick
>