[m-rev.] diff/review: deep profiler enhancements

[Zoltan Somogyi]

On 03-Jul-2001, Fergus Henderson <fjh at cs.mu.OZ.AU> wrote:
> There should be some comments here - at very least copy the comments
> about this file from the log message.

I will add not just this comment but many others next week. I wanted to commit
this stuff today.

> It would be nice to have a couple of comments here indicating what colors
> these are, e.g.
> 
> 	even_background = "rgb(255, 255, 240)". % light magenta
> 	odd_background =  "rgb(240, 240, 255)". % hot pink
> 
> (except that I made up those colors names without regard to the rgb values).

Tom gave me the rgb values. I can't even tell what colour they are, because
they don't display on miles.

> Get rid of the `#define MDPROF_NUM_SIGNAL_NUMBERS 12'
> and replace it with
> 
> 	extern const int	signal_numbers[];
> 	extern const int	MDPROF_num_signal_numbers;
> 
> and
> 
> 	#define MDPROF_ARRAY_LEN(a) (sizeof(a)/sizeof((a)[0]))
> 
> 	const int signal_numbers[] = { ... }
> 	const int MDPROF_num_signal_numbers = MDPROF_ARRAY_LEN(signal_numbers);

I originally did that. I switched to the current unsatisfactory scheme when
code very like the above caused gcc to abort with the usual message about
spilling a forbidden register, and simple code transformations failed to work
around the problem.

> By the way, shouldn't all the externally visible C identifiers in the
> deep profiler (e.g. `signal_numbers') have a prefix (e.g. `MDPROF_') on them?

These identifiers really shouldn't be external; all the code using them is in
this module. The only reason they are declared extern is to declare them for
other C code fragments in the *same* module without code duplication.
There should be a mechanism to inhibit --split-c-files and intermodule
inlining for modules like this that want to preserve an abstraction boundary
for their C code.

However, in the short term I will add the prefix.

> > +	void	(*handler)(void);
> 
> This is a type error; the arguments types to handler() don't
> match the types that signal() or sigaction() expects.
> It happens to work on many C implementations, but it's not
> guaranteed to work.

As I recall, I don't think there is consistency about various operating
system flavours about the arguments passed to signal handlers. I would
love to be proven wrong on this, though.

Zoltan.
--------------------------------------------------------------------------
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
--------------------------------------------------------------------------