RFA: Improve message for wrong number of alternatives

[Richard Sandiford]

Joern Rennecke <joern.rennecke@embecosm.com> writes:
> On Sun, 16 May 2021 at 22:01, Martin Sebor <msebor@gmail.com> wrote:
>  > I think it's very helpful to provide this sort of detail.  Just as
>> a matter of readability, the new error message
>>
>>    "wrong number of alternatives in operand %d, %d, expected %d"
>>
>> would be improved by avoiding the two consecutive %d's,
>
> We could also do that by phrasing it:
>
> "wrong number of alternatives in operand %d, seen: %d, expected: %d"
>
> so that the change is just about adding extra information.
>
>> e.g., by
>> rephrasing it like so:
>>
>>    "%d alternatives provided to operand %d where %d are expected"
>
> This has an additional change in that we no longer jump to the conclusion
> that the operand where we notice the discrepancy is the point that's wrong.
> I suppose that conclusion is more often right than wrong (assuming more than
> two operands on average for patterns that have alternatives and at least two
> operands), but when it's wrong, it's particularly confusing and/or jarring,
> so it's an improvement to just stick to the known facts.
> But if we go that way, I suppose we should spell also out where the
> expectation comes from: we have a loop over the operands, and we look at
> operand 0 first.  We could do that by using the diagnostic:
>
>               error_at (d->loc,
>                         "alternative number mismatch: operand %d has
> %d, operand %d had %d",
>                         start, d->operand[start].n_alternatives, 0, n);
>
>
> I notice in passing here that printf is actually awkward for repharasings
> and hence also for translations, because we can't interchange the order of
> the data in the message string.
>
> But for multi-alternative patterns, we also have the awkwardness of
> repeating the abstract of the error message and the recap of the number
> of alternatives of operand 0.
>
> So I propose the attached patch now.
>
> Bootstrapped on x86_64-pc-linux-gnu.
>
> 2021-05-17  Joern Rennecke  <joern.rennecke@embecosm.com>
>
> 	Make "wrong number of alternatives" message more specific, and
> 	remove assumption on where the problem is.
>
> diff --git a/gcc/genoutput.c b/gcc/genoutput.c
> index 8e911cce2f5..6313b722cf7 100644
> --- a/gcc/genoutput.c
> +++ b/gcc/genoutput.c
> @@ -757,6 +757,7 @@ validate_insn_alternatives (class data *d)
>  	int which_alternative = 0;
>  	int alternative_count_unsure = 0;
>  	bool seen_write = false;
> +	bool alt_mismatch = false;
>  
>  	for (p = d->operand[start].constraint; (c = *p); p += len)
>  	  {
> @@ -813,8 +814,19 @@ validate_insn_alternatives (class data *d)
>  	    if (n == 0)
>  	      n = d->operand[start].n_alternatives;
>  	    else if (n != d->operand[start].n_alternatives)
> -	      error_at (d->loc, "wrong number of alternatives in operand %d",
> -			start);
> +	      {
> +		if (!alt_mismatch)
> +		  {
> +		    alt_mismatch = true;
> +		    error_at (d->loc,
> +			      "alternative number mismatch: "
> +			      "operand %d had %d, operand %d has %d",
> +			      0, n, start, d->operand[start].n_alternatives);

IMO this is better with s/had/has/.  OK with that change, thanks.

Richard


> +		  }
> +		else
> +		  error_at (d->loc, "operand %d has %d alternatives",
> +		    start, d->operand[start].n_alternatives);
> +	      }
>  	  }
>        }
>