[m-rev.] for review: updates for opengl binding

[Fergus Henderson]

On 14-Aug-2003, Julien Fischer <juliensf at students.cs.mu.OZ.AU> wrote:
> 
> extras/graphics/mercury_opengl/mglu.m:
> extras/graphics/mercury_opengl/mogl.m:
>         Use the new foreign language interface instead of pragma c_code.

Good.

>         Use state variables instead of DCGs for threading the IO state.

I suppose that is OK, although I'm not convinced that it improves
readability in most of these cases.

>         Use `.' as a module qualifier rather than `__' or `:'.

Support for "." as a module qualifier is a fairly recently addition.
In general I would recommend that packages in mercury-extras should
avoid depending on Mercury features that are not yet present in the
latest public release of Mercury if it is not necessary.
So please don't do this to any more packages in the extras directory
until Mercury 0.12 is released.

However, since you have already done it for this package, you may as
well go ahead and commit it (once you have addressed the other review
comments, of course).

>         Add some missing MR_* prefixes.

For Float -> MR_Float, fine and good.
For assert() -> MR_assert(), this is probably not appropriate --
see my comment below.

>         Add end_module declarations.

Fine.

>         Add a comment about the status of the binding with respect to
>         OpenGL versions 1.2 - 1.4.

Good.

>         Remove comments referring to section numbers in version 1.1 of
>         the OpenGL spec.

Removing the section numbers, fine. 
But please don't remove the comments themselves.

>         Make the formatting and layout of the modules consistent.

Good... but please also make sure that the formatting and layout
of the modules is consistent with the code elsewhere in the Mercury
implementation.  Such layout conventions should be documented in the
Mercury coding standards (compiler/notes/coding_standards.html).

>         Add some missing const qualifiers.

Good.

>         Remove predicate mogl.make_mask/3 and use predicates from the std.
>         library to implement it instead.

Fine.

> Index: mogl.m
>  %------------------------------------------------------------------------------%
> -%
> -% 2.5	GL Errors
> -%
> -%------------------------------------------------------------------------------%
> -

In the implementation section of this module,
you have removed the section heading comments entirely.
I don't think that is a good idea -- I find the section
headings useful.  So please don't remove them.

>  %------------------------------------------------------------------------------%
> -%
> -% 5.5 Flush and Finish
> -%
> -%------------------------------------------------------------------------------%
> 
> -:- pragma c_code(flush(IO0::di, IO::uo), "
> +:- pragma foreign_proc("C", flush(IO0::di, IO::uo),
> +	[will_not_call_mercury, promise_pure], "
>  	glFlush();
>  	IO = IO0;
> -	assert(glGetError() == GL_NO_ERROR);
> +	MR_assert(glGetError() == GL_NO_ERROR);
>  ").
> 
> -:- pragma c_code(finish(IO0::di, IO::uo), "
> +:- pragma foreign_proc("C", finish(IO0::di, IO::uo),
> +	[will_not_call_mercury, promise_pure], "
>  	glFinish();
>  	IO = IO0;
> -	assert(glGetError() == GL_NO_ERROR);
> +	MR_assert(glGetError() == GL_NO_ERROR);
>  ").

MR_assert() is by default turned off.  So these error conditions won't
get checked for.  I don't think that is a good idea (at least not without
a comment explaining why they won't occur, if in fact that is the case).

> -enable(Flag) -->
> -	( { Flag = clip_plane(I) } ->
> -		enable3(control_flag_to_int(Flag), I)
> -	; { Flag = light(I) } ->
> -		enable3(control_flag_to_int(Flag), I)
> -	;
> -		enable2(control_flag_to_int(Flag))
> +enable(Flag, !IO) :-
> +	( if		Flag = clip_plane(I)
> +	  then		enable3(control_flag_to_int(Flag), I, !IO)
> +	  else if	Flag = light(I)
> +	  then		enable3(control_flag_to_int(Flag), I, !IO)
> +	  else		enable2(control_flag_to_int(Flag), !IO)
>  	).
...
> -disable(Flag) -->
> -	( { Flag = clip_plane(I) } ->
> -		disable3(control_flag_to_int(Flag), I)
> -	; { Flag = light(I) } ->
> -		disable3(control_flag_to_int(Flag), I)
> -	;
> -		disable2(control_flag_to_int(Flag))
> +disable(Flag, !IO) :-
> +	( if		Flag = clip_plane(I)
> +	  then 		disable3(control_flag_to_int(Flag), I, !IO)
> +	  else if 	Flag = light(I)
> +	  then		disable3(control_flag_to_int(Flag), I, !IO)
> +	  else		disable2(control_flag_to_int(Flag), !IO)
>  	).

The layout of the if-then-elses here isn't consistent with the layout
used in compiler/*.m.

The Mercury coding guidelines currently don't discuss the
"(if ... then ... else ...)"  form of if-then-else, instead
specifying guidelines only for the "(... -> ... ; ... )" form.
But if you're going to reformat if-then-elses to use a different syntax,
you should first make sure there is a documented agreement on how they
should be formatted.

Personally, I don't particularly like the layout that you've chosen,
especially because it uses two levels of indentation, which if used
consistently would result in too much unnessary line wrapping and
would get exceedingly messy for nested if-then-elses.  I'd prefer to use

	disable(Flag, !IO) :-
		(if Flag = clip_plane(I) then 	
			disable3(control_flag_to_int(Flag), I, !IO)
		else if Flag = light(I) then
			disable3(control_flag_to_int(Flag), I, !IO)
		else
			disable2(control_flag_to_int(Flag), !IO)
		).

which is IMHO more consistent with the style that we use for the ->; form,


The rest of this diff looks fine.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
mercury-reviews mailing list
post:  mercury-reviews at cs.mu.oz.au
administrative address: owner-mercury-reviews at cs.mu.oz.au
unsubscribe: Address: mercury-reviews-request at cs.mu.oz.au Message: unsubscribe
subscribe:   Address: mercury-reviews-request at cs.mu.oz.au Message: subscribe
--------------------------------------------------------------------------