Joern Rennecke <joern.renne...@embecosm.com> writes:
> On Sun, 16 May 2021 at 22:01, Martin Sebor <mse...@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.renne...@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);
> + }
> }
> }
>
