[PATCH v5] Add pretty printers for the NPTL lock types

[Martin Galvan]

On Mon, Apr 11, 2016 at 5:38 PM, Roland McGrath <roland@hack.frob.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
    import a-b.py
            ^
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:

https://sourceware.org/ml/libc-alpha/2016-04/msg00179.html

> 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
particular patch.

> +$(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
want to.

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

Will do.

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