This is the mail archive of the
mailing list for the glibc project.
Re: [PATCH v5] Add pretty printers for the NPTL lock types
- From: Martin Galvan <martin dot galvan at tallertechnologies dot com>
- To: Roland McGrath <roland at hack dot frob dot com>
- Cc: libc-alpha at sourceware dot org, Daniel Gutson <daniel dot gutson at tallertechnologies dot com>, "Carlos O'Donell" <carlos at redhat dot com>, Siddhesh Poyarekar <sid at reserved-bit dot com>, Tom Tromey <tom at tromey dot com>
- Date: Mon, 11 Apr 2016 17:52:03 -0300
- Subject: Re: [PATCH v5] Add pretty printers for the NPTL lock types
- Authentication-results: sourceware.org; auth=none
- References: <1460405322-31278-1-git-send-email-martin dot galvan at tallertechnologies dot com> <20160411203830 dot EF45B2C3B22 at topped-with-meat dot com>
On Mon, Apr 11, 2016 at 5:38 PM, Roland McGrath <email@example.com> wrote:
> I don't understand what the new top-level subdir is for.
> The new code should go in the subdir defining the types, i.e. nptl.
Funny, that's the way I originally had it but Sid asked me to move
everything to a top-level subdir. I personally like it better this
way, especially after I introduced the test suite.
> We don't normally use _ in file names in libc, we only use -.
> Why are some of the new files named with _s?
AFAIK Python requires it:
>>> import a-b.py
File "<stdin>", line 1
SyntaxError: invalid syntax
> I noticed several lines (perhaps all comments) that were a bit too long.
> No line should have column 80 occupied.
We discussed this with Sid, here:
> In some places in the comments, you used only one space between sentences.
> We use two spaces between sentences everywhere.
Really? I though I had fixed them all. Could you point out where?
> PYTHON should be set in Makeconfig, not in a random Makefile. It should
> probably be found by configure and substituted in config.make, in fact.
> That lets people point to a particular binary at configure time.
Yeah, I agree. I can add something like:
PYTHON := python
to Makeconfig, but I'm not messing with configure scripts for this
> +$(py-const): $(py-const-dir)%.py: %.pysym $(py-const-script) \
> + $(common-before-compile)
> + $(make-target-directory)
> + $(AWK) -f $(py-const-script) $*.pysym \
> + | $(CC) -S -o $@.tmp $(CFLAGS) $(CPPFLAGS) -x c -
> + echo '# GENERATED FILE\n' > $@.tmp2
> + echo '# Constant definitions for pretty printers.' >> $@.tmp2
> + echo '# See gen-py-const.awk for details.\n' >> $@.tmp2
> This should use $< in place of $*.pysym.
I think $*.pysym eases understanding of that line, and it doesn't
depend on the order of the prerequisites. But I can change it if you
> + sed -n -r 's/^.*@name@([^@]+)@value@[^[:xdigit:]Xx-]*([[:xdigit:]Xx-]+)@.*/\1 = \2/p' \
> + $@.tmp >> $@.tmp2
> + mv -f $@.tmp2 $@
> + rm -f $@.tmp
> Drop all the blank lines inside the command sequence. They make it
> confusing as to what is part of the same rule or not.
> +before-compile += $(py-const)
> Surely these don't need to be in before-compile. That is only for things
> that must exist before C code can be compiled.
Good point. Will remove it.
> All new files must start with a descriptive comment as their first line,
> before the copyright notice.
All my files have a descriptive comment, though it's after the
copyright notice. I can move it up if you want to.
> I have not reviewed any of the actual C or Python code.
It's been reviewed already, but feel free to throw in some feedback if you like.