[m-rev.] for review: improve error reporting for foreign_enum pragmas

[Zoltan Somogyi]

> 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
entities.

> +        % 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?

> +        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”.

The rest is fine.

Zoltan.