On Mar 9 16:54, Mark Geisert wrote: > 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.
Thanks. > 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!! :-). Haha! But, seriosly, indent(1) is a wonderful tool for that. With the right options there's no subsequent manual work. Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Maintainer cygwin AT cygwin DOT com Red Hat
signature.asc
Description: PGP signature