[m-rev.] for review: Reduce stack consumption in lexer.

[Zoltan Somogyi]

On Wed, 14 Oct 2015 13:25:45 +1100, Peter Wang <novalazy at gmail.com> wrote:
> library/lexer.m:
> 	Make `get_token_2' take a scanned_past_whitespace parameter.

The comments say you are relying on the inlining of get_token_2
into get_token to get tail recursion, but some of the grades that
don't normally allow tail recursion also inhibit inlining, so I don't think
this approach will work in every case. What are the use cases whose
stack consumption you are worrying about?

> +    % We defer calls to `execute_get_token_action' from its descendents until
> +    % we have returned to `execute_get_token_action'.

DescendAnts. Also, the above is unclear unless you already know what
it is talking about. Partly this is because you don't explicitly differentiate
the original call to execute_get_token_action, the indirectly recursive call
to that predicate that you want to avoid, and the directly recursive call
you want to replace the second call with. Partly this is because you don't say
*how* the deferral is done; *how* does returning "no" defer the call?
What is your intended the call graph of the relevant predicates?

> +    % We nominally define :- type have_token == maybe(token_context) and
> +    % defer a call by returning `no'. To avoid heap allocation we drop the
> +    % the `maybe' wrapper and represent `no' as a value with an invalid
> +    % context (-1).

This is not what "we nominally define" means. I suggest "we would like to define
":- type have_token == maybe(token_context)", but the heap allocation required
to return yes(Context) would be a significant overhead, so we define <the actual
definition>, and represent "no" as an invalid context, i.e. one for which
have_token_with context fails."

> +:- type have_token
> +    --->    have_token(token_context).

I would rename the type and functor as maybe_have_valid_token.

For the predicates that take a pair of a token and a have_token (or whatever
its name ends up being), please document that if have_token_with_context
fails on the latter, then the former is not meaningful. (If it *is* meaningful,
then I misunderstood what you are doing, and more documentation would be
needed to avoid such misunderstandings :-()

> +:- pred have_token(io.input_stream::in, have_token::out, io::di, io::uo)
> +    is det.

Please give the predicate a different name than the type.

I can't tell whether the rest of the diff correctly implements your intended
call graph until you describe what it is, but if it passes bootcheck, I am happy
to look at it again post commit.

Zoltan.