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

[Roland McGrath <roland at hack dot frob dot com>]

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.

We don't normally use _ in file names in libc, we only use -.
Why are some of the new files named with _s?

I noticed several lines (perhaps all comments) that were a bit too long.
No line should have column 80 occupied.

In some places in the comments, you used only one space between sentences.
We use two spaces between sentences everywhere.

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.

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

+
+# Replace "@name@SOME_NAME@value@SOME_VALUE@" strings from the output of
+# 'gcc -S' with "SOME_NAME = SOME_VALUE" strings.
+# The '-n' option, combined with the '/p' command, makes sed output only the
+# modified lines instead of the whole input file. The output is redirected
+# to a .py file; we'll import it in the pretty printers file to read
+# the constants generated by gen-py-const.awk.
+# The regex has two capturing groups, for SOME_NAME and SOME_VALUE
+# respectively. Notice SOME_VALUE may be prepended by a special character,
+# depending on the assembly syntax (e.g. immediates are prefixed by a '$'
+# in AT&T x86, and by a '#' in ARM). We discard it using a complemented set
+# before the second capturing group.
+	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.

All new files must start with a descriptive comment as their first line,
before the copyright notice.

I have not reviewed any of the actual C or Python code.