Dmitrij Pochepko <dmitrij.poche...@bell-sw.com> writes:
> Hi,
>
> thank you for reviewing it.
>
> Please check updated version(attached) with all comments addressed.
>
> Thanks,
> Dmitrij
>
> On Tue, Jun 23, 2020 at 06:10:52PM +0100, Richard Sandiford wrote:
> ...
>> 
>> I think it would be better to test this as part of the loop below.
>> 
> done

For this point, I meant that we should remove the first loop too.  I.e.:

> +  unsigned int encoded_nelts = d->perm.encoding ().encoded_nelts ();
> +  for (unsigned int i = 0; i < encoded_nelts; ++i)
> +    if (!d->perm[i].is_constant ())
> +      return false;

is now redundant with the later:

> +  for (unsigned HOST_WIDE_INT i = 0; i < nelt; i ++)
> +    {
> +      poly_int64 elt = d->perm[i];
> +      if (!elt.is_constant ())
> +     return false;
> +      if (elt.to_constant () == (HOST_WIDE_INT) i)
> +     continue;

However, a more canonical way to write the condition above is:

  for (unsigned HOST_WIDE_INT i = 0; i < nelt; i++)
    {
      HOST_WIDE_INT elt;
      if (!elt.is_constant (&elt))
        return false;
      if (elt == (HOST_WIDE_INT) i)
        continue;

Very minor, but the coding conventions don't put a space before “++”.
So:

> +      for (unsigned HOST_WIDE_INT i = 0; i < nelt; i ++)

…this should be “i++” too.

Looks good otherwise, thanks.

Richard

Reply via email to