[m-rev.] for post-commit review: color in add_pragma.m

[Zoltan Somogyi]

On 2024-05-12 23:19 +10:00 AEST, "Julien Fischer" <jfischer at opturion.com> wrote:
>> --- a/tests/invalid/bug83.err_exp
>> +++ b/tests/invalid/bug83.err_exp
>> @@ -1,7 +1,7 @@
>> -bug83.m:027: Error: ambiguous function name in `:- pragma foreign_export'
>> +bug83.m:027: Error: ambiguous function name in `:- pragma foreign_export'
> 
> I suggest using color_subject on ":- pragma foreign_export".

In this case, the phrase "in pragma foreign_export" plays the same role
as the "In this argument of this syntactic construct:" preamble that
we have on many other kinds of error messages. I think it would be
an inconsistency if we colored one but not the other.

I could reword that message to move that context up front, so that
it would read: In pragma foreign_export declaration: ambiguous function name, ..."
Would that be better?
 
>> --- a/tests/invalid/inline_conflict.err_exp
>> +++ b/tests/invalid/inline_conflict.err_exp
>> @@ -1,2 +1,2 @@
>> -inline_conflict.m:019: Error: `:- pragma no_inline' declaration conflicts with
>> -inline_conflict.m:019:   previous inline pragma for `bar'/2.
>> +inline_conflict.m:019: Error: `:- pragma no_inline' declaration conflicts with
>> +inline_conflict.m:019:   previous inline pragma for `bar'/2.
> 
> Unrelated: it might be nice to give the context for the previous inline pragma
> in the message as well.

Yes, it would be nice. However, the predicate that generates that
message, add_pred_marker in add_pragma.m, does not have that
context, because the HLDS has no slot for that information.
We could add it, but this error is so rare, and the context
of the conflicting pragma is so trivially easy to find for a human,
that noone has never thought it worthwhile to make that change.

> The rest looks fine.

Thank you very much, for  this and for all the other reviews.

Zoltan.