[m-rev.] for review: diagnose non-contiguous clauses

[Julien Fischer]

Hi,

On Mon, 17 Aug 2009, Zoltan Somogyi wrote:

> For review by anyone. Anyone have better names for the options that aren't
> excessively long?

The names do not appear to be excessively long, at least in comparison
to some of those that already exist.

> Add a mechanism for generating warnings when the various clauses of a predicate
> or function are not contiguous.
>
> This mechanism consists of two options:
>
> 	--warn-non-contiguous-clauses
> 	--warn-non-contiguous-foreign-procs
>
> The first option generates warnings when the Mercury clauses of a predicate
> or function are not contiguous, but it ignores any foreign_procs of that
> predicate or function, and thus allows these to be away from the Mercury
> clauses and each other. This option is enabled by default.

That's fine, but until the installed compilers understand the new
options can you please disable it by default.

> The second option generating warnings unless both the Mercury clauses and
> all the foreign_procs of the predicate or function are all contiguous.
> This option is not enabled by default, because many library modules
> group foreign_procs not by predicate, but by foreign language. (All C foreign
> procs for a group of predicates, then all the Java foreign procs for that group
> of predicates, etc.)
>
> compiler/hlds_clauses.m:
> 	Store, next to the representation of each clause list, information
> 	about the locations (item numbers and context) of the clauses.
> 	We store two versions of this information, one version for each option.
>
> 	Make the predicates that access the clause list access the location
> 	information as well, to ensure that any code that adds clauses also
> 	records their location.
>
> 	Add a predicate that tests for non-contiguity.
>
> 	Add a specific type to represent the modes that a clause applies to.
> 	This replaces the error-prone scheme we used to use that represented
> 	the notion "this clause applies to all modes" with an empty list of
> 	modes. This allows us to remove the code in add_pragma.m that used
> 	to replace these empty lists with the list of actual modes they
> 	represented.
>
> 	Change the prefix on the fields of clauses_info to avoid ambiguities.
> 	Add a prefix to the names of the function symbols of the clauses_rep
> 	type to avoid ambiguities.
>
> compiler/add_clause.m:
> compiler/add_pragma.m:
> 	When adding Mercury clauses and pragma foreign_procs to a predicate or
> 	function, record the location of the clause or foreign_procs. We do so
> 	even if the clause or foreign_proc is overridden by another. For
> 	example, when compiling to C, a Mercury clause overrides an Erlang
> 	foreign_proc, and a C foreign_proc overrides a Mercury clause.
>
> 	Fix an old bug where a foreign_proc that should override Mercury
> 	clauses overrode only one Mercury clause, and left the others
> 	in the predicate, to yield a disjunction with some Mercury disjuncts
> 	and a foreign_proc disjunct. This disjunction would then yield
> 	determinism errors if it had outputs.
>
> 	The new code that fixes the bug has a much more direct implicit
> 	correctness argument, and should be significantly easier to understand.
> 	It also avoids doing unnecessary work. (The old code could make a
> 	decision to ignore a clause, yet still proceed to transform it,
> 	just to ignore the result of the transformation.)

WRT to the third part, the compiler's behaviour is still undesirable
here.  It should really delay committing to a set of clause definitions for
a predicate until after semantic checking has been completed.  (If we
did this then we should not omit transforming the clause -- actually does
omitting the transformation weaken existing error checks of alternative
defintions?)

Currently, it is possible to write a C foreign proc for a Mercury
predicate and, say a type-incorrect, Mercury clause for it and have that
"successfully" compile until one attempts to compile in a non-C grade.
Throwing alternative definitions away causes other problems as well,
e.g. the XXX comment at the head of compiler/unused_imports.m.

I am not saying that this should be fixed as part of this change, but
it is worth bearing in mind that the "let's throw some of the
definitions away early" approach will eventually need to change.

The diff itself looks fine.

Julien.
--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews at csse.unimelb.edu.au
Administrative Queries: owner-mercury-reviews at csse.unimelb.edu.au
Subscriptions:          mercury-reviews-request at csse.unimelb.edu.au
--------------------------------------------------------------------------