[m-rev.] for review: improve error reporting for foreign_enum pragmas
jfischer at opturion.com
Thu May 19 23:26:26 AEST 2016
On Thu, 19 May 2016, Zoltan Somogyi wrote:
>> On 18 May 2016, at 11:22, Julien Fischer <jfischer at opturion.com> wrote:
>> @@ -737,42 +725,72 @@ parse_export_enum_attr(VarSet, Term, MaybeAttribute) :-
>> parse_pragma_foreign_enum(VarSet, ErrorTerm, PragmaTerms, Context, SeqNum,
>> MaybeIOM) :-
>> ( if PragmaTerms = [LangTerm, MercuryTypeTerm, ValuesTerm] then
>> ( if parse_foreign_language(LangTerm, ForeignLang) then
>> - parse_export_enum_type_ctor(MercuryTypeTerm, MaybeTypeCtor),
>> + MaybeForeignLang = ok1(ForeignLang)
>> + else
>> + LangPieces = [words("Error: invalid foreign language in"),
>> + pragma_decl("foreign_enum"), words("declaration."),
>> + nl],
>> + LangSpec = error_spec(severity_error, phase_term_to_parse_tree,
>> + [simple_msg(get_term_context(LangTerm), [always(LangPieces)])]),
>> + MaybeForeignLang = error1([LangSpec])
>> + ),
> I think it could help if the message said e.g. "invalid foreign language `charp’
> in …”, so I would call describe_error_term on LangTerm.
>> + parse_type_ctor_name_arity("foreign_enum", MercuryTypeTerm,
>> + MaybeTypeCtor),
>> + UnrecognizedPieces =
>> + [words("Error: expected a valid mapping element")],
> I would say explicitly what the "mapping element” is supposed
> to be, i.e. what it maps to what. I would do that by giving a template,
> since it is not obvious e.g what should separate the source and target
I'll do that as a separate change -- I was intending to rework the code
for parsing the third argument of foreign_enum pragmas so we could
return more accurate error messages anyway. (The existing code has
a tendency to simply fail if it detects an error and output the above
>> + % XXX the following doesn't check that foreign values are sensible
>> + % (e.g. it should reject the empty string).
> Would it be easy to add that check?
As part of the above reworking, yes.
>> + convert_maybe_list("mapping elements", yes(VarSet), ValuesTerm,
>> + parse_sym_name_string_pair(VarSet, PairContextPieces),
>> + UnrecognizedPieces, MaybeValues0),
>> + (
>> + MaybeValues0 = ok1(Values),
> The convention I have been using for code like this, where the initial
> part of the code conditionally puts a value into a maybe1 and the later
> part takes it out again, is to name the first appearance of the variable
> a 0 suffix. So I would rename Values to Values0, and the later ValuesPrime
> to plain Values.
>> diff --git a/tests/invalid/invalid_pragma.err_exp b/tests/invalid/invalid_pragma.err_exp
>> new file mode 100644
>> index 0000000..6a9fe32
>> --- /dev/null
>> +++ b/tests/invalid/invalid_pragma.err_exp
>> @@ -0,0 +1,2 @@
>> +invalid_pragma.m:006: Error: a `:- pragma' declaration should have the form
>> +invalid_pragma.m:006: `:- pragma pragma_name(pragma_arguments).’
> It may help if this continued “`1235’ is not a valid/recognized pragma name”.
I'll look at this one separately too.
More information about the reviews