This is the mail archive of the cygwin-patches mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On Wed, 9 Mar 2016, Corinna Vinschen wrote:
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?

The "long" was a dumb mistake (and malingering 32-bit orientation) on my part. It shall be made int32_t of course.

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. */

I'll go over the whole patch set to get rid of nonleading TABs and to fix the comments as you suggest. The latter was just me not realizing there was a convention to follow so I didn't :-). All shall be fixed.

gmon.c/.h need to be reformatted to GNU style like the rest of Cygwin but I'm leaving that to a future patch (NO I AM NOT COMMITTING TO THAT!! :-).

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


Cheers,

..mark


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]