Re: [PATCH] Support profiling of multi-threaded apps.

[Corinna Vinschen <corinna-cygwin at cygwin dot com>]

Hi Mark,

On Mar  9 00:39, Mark Geisert wrote:
> This is Version 3 incorporating review comments of Version 2.  This is just
> the code patch; a separate doc patch is forthcoming.

The patch looks fine to me code-wise.  I just have a few style requests:

> +	if ((prefix = getenv("GMON_OUT_PREFIX")) != NULL) {
> +		char *buf;
> +		long divisor = 1000*1000*1000;	// covers positive pid_t values

Why "long"?  It's safe to use here, but it doesn't match the incoming
datatype.  pid_t is 4 bytes, but long is 8 bytes on x86_64.  If you
like it better that way we can keep it in but wouldn't, say, int32_t
be a better match?  Also, can you convert the TAB to a space preceeding
the comment so it's within 80 columns, please?

I'm also a bit unhappy with some of the comments not trailing on a code
line.  E.g.:

// sample the pc of the thread indicated by thr, but bail if anything amiss

// record profiling samples for other pthreads, if any

Ideally that would be /**/ style comments, starting in uppercase and
as full sentences ending with a dot, e.g.:

/* Sample the pc of the thread indicated by thr, but bail if anything amiss. */

/* Record profiling samples for other pthreads, if any. */

With these changes I'll apply the patch.


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

Attachment: signature.asc
Description: PGP signature