[m-rev.] for post-commit review: fix infinite loop bug in compiler

Julien Fischer jfischer at opturion.com
Wed Oct 13 13:07:58 AEDT 2021


Hi Zoltan,

On Mon, 11 Oct 2021, Zoltan Somogyi wrote:

> For review by anyone.
>
> I also have some questions raised by this diff.
>
> Q1: if two or more insts, say i1. i2 and i3 are mutually recursive, then
> by definition, the definitions of all of them are circular. After this diff,
> we generate one error message for each such inst. Should we instead
> generate a single error message for the cycle as a whole?

Ideally, yes.

> Q1a: if the answer to Q1 is yes, then is it enough for the one error message
> to contain the context of just one inst (either the context with the smallest
> line number, or the context for the lexicographically first inst name), or
> should that one error message have one component for each inst giving
> its context?
>
> My preferred answer to  Q1 is yes. I am agnostic on Q1a, primarily because
> (a) the contexts of some insts may refer to .intN files, which I don't think we
> want to print, but (b) non-.intN contexts would be useful, and (c) printing
> only the non-.intN contexts would look strange.

Rather than printing the .intN contexts, we could just give the context
of the insts in the module being compiled and say something like:

       ... and the imported insts `foo' and `bar'.


(i.e. we would have two different forms of the error message dependent
on whether it refers to an imported insts / modes.)

> Q2: At the moment, all the inst names printed in error messages are fully
> module qualified. This is definitely overkill for the inst being complained about,
> if it is locally defined. I see three alternatives:
>
> A2a: report all insts in fully qualified form, as now.
> A2b: report all insts in unqualified form.
> A2c: report all insts from the current module in unqualified form,
> and all other insts in fully qualified form.
> A2d: report all insts from the current module *that are not also defined
> in builtin.m* in unqualified form, and all other insts in fully qualified form.
>
> The point of A2d is that insts and modes defined in builtin.m are
> the only insts/modes whose form in error messages is the same as their
> unqualified form, since we routinely strip that module from module
> qualifications.
>
> My preferred answer to Q2 is A2d.

As is mine.

> Q3: Instead of the current terse error message, which has the form
> "circular equivalence {inst 1,insts i1 and i2}", should we have a longer
> error message that explains *why* circularity is a problem?
>
> My preferred answer is yes. I would prefer wording such as
> "Inst name i1 expands to an inst containing itself, which means that
> processing any reference to i1 would require an infinite sequence
> of expansions", adding "through insts i2 and i3" after "expands to"
> in the case of mutual recursion. Note that we can't say "would expand
> to an infinitely large inst", because e.g. ":- inst inf == inf" wouldn't.

That wording seems fine.

The diff is fine.

Julien.


More information about the reviews mailing list