[m-dev.] for review: add the POSIX stuff to the extras

[Fergus Henderson]

Hi Tom,

Here's some review comments on the extras/posix stuff.

> Index: Mmake

You should use `Mmakefile' rather than `Mmake', to avoid
problems on case-insensitive file systems where "." is first
in the PATH.

> +MLLIBS = posix_workarounds.o

You should use MLOBJS and MLPICOBJS rather than MLLIBS,
so that it works if you try to build a shared library.

> diff -N hello.m
> +:- pred main(io:state, io:state).
> +:- mode main(di, uo) is det.

You should use `io__state' rather than `io:state',
so that it works when `:' becomes the type qualifier.

> +:- import_module posix, posix:open, posix:write, text.

Likewise you should use `posix__open' rather than `posix:open'.

> Index: posix.lseek.m
...
> +:- pragma c_code(whence(W::in) = (V::out),
> +		[will_not_call_mercury, thread_safe], "{
> +	static int whence_flags[] = { SEEK_SET, SEEK_CUR, SEEK_END } ;
> +	V = whence_flags[W];
> +}").

s/static/static const/

> +:- pragma c_code(oflagval(F::in) = (V::out),
> +		[will_not_call_mercury, thread_safe], "{
> +	static int oflag_values[] = {
> +		O_RDONLY, O_WRONLY, O_RDWR, O_CREAT, O_EXCL, O_NOCTTY,
> +		O_TRUNC, O_APPEND, O_NDELAY, O_SYNC };

Likewise here.  Also it would be better IMHO to put the "};"
on a new line, indented the same as the "static int ..." line.

> +:- pragma c_code(domain(D::in) = (V::out),
> +		[will_not_call_mercury, thread_safe], "{
> +	static int domain_values[] = {
> +		AF_UNIX, AF_INET
> +	};

Likewise here.

> +:- pragma c_code(type(T::in) = (V::out),
> +		[will_not_call_mercury, thread_safe], "{
> +	static int type_values[] = {
> +		SOCK_STREAM, SOCK_DGRAM, SOCK_RAW, SOCK_SEQPACKET, SOCK_RDM
> +	};

And here.

> +:- pragma c_code(mkinet_addr(A::in, P::in, Ptr::out, Len::out), 
> +		[will_not_call_mercury, thread_safe], "{
> +	struct sockaddr_in *ptr;
> +
> +	incr_hp(Ptr, (1 + sizeof(struct sockaddr_in)/sizeof(Word)));
> +	ptr = (struct sockaddr_in *) Ptr;
> +
> +	memset((void *) ptr, 0, sizeof(struct sockaddr_in));

The case to (void *) here is unnecessary, and it does not aid
readability IMHO.

> +:- module text.
...
> +:- func text(string) = text.
> +:- mode (text(in) = uo) is det.

The parentheses around `text(in) = uo' are unnecessary,
and IMHO harm readability.

> +:- func unique(text) = text.
> +:- mode (unique(in) = uo) is det.

Likewise here.

> +	/*
> +	** ME_words(amt) returns the number of words necessary to
> +	** to store `amt' bytes.
> +	*/
> +	#define ME_words(x)	(1+(x)/sizeof(Word))

That formula allocates more memory than is needed.
The correct formula here is `(((x) + sizeof(Word) - 1) / sizeof(Word))'.

Also there are a lot of places where you hard-coded that formula,
where you should instead use a macro like that one.
(You could put the macro in runtime/mercury_memory.h.)

> +:- pragma c_code(index(Txt::ui, Ind::in, Val::out),
> +		[will_not_call_mercury, thread_safe], "{
...
> +%------------------------------------------------------------------------------%
> +
> +:- pragma c_code(index(Txt::in, Ind::in, Val::out),
> +		[will_not_call_mercury, thread_safe], "{

I suggest you delete the line between those two diffent modes for index/3.

Other comments:
	- the type `fdset' really represents a pointer, not a set,
	  so IMHO it should be named `fdset_ptr'.
	- the interface to the `text' module should be documented better
	- an interface to strerror() would be very handy

Apart from that, it looks good.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
WWW: <http://www.cs.mu.oz.au/~fjh>  |  of excellence is a lethal habit"
PGP: finger fjh at 128.250.37.3        |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
mercury-developers mailing list
Post messages to:       mercury-developers at cs.mu.oz.au
Administrative Queries: owner-mercury-developers at cs.mu.oz.au
Subscriptions:          mercury-developers-request at cs.mu.oz.au
--------------------------------------------------------------------------