This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc 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] |
On 09 Oct 2015 19:24, Martin Galvan wrote: > --- a/Makerules > +++ b/Makerules > > +ifdef gen-py-const-headers > +# This is a hack we use to generate .py files with constants for Python > +# pretty-printers. It works the same way as gen-as-const. See gen-py-const > +# for details on how the awk | gcc mechanism works. > +$(common-objpfx)%.py: $(..)scripts/gen-py-const.awk %.pysym $(common-before-compile) > + $(AWK) -f $< $(filter %.pysym,$^) this duplicated line left in by accident for debugging ? should delete it. > + echo '# GENERATED FILE, DO NOT EDIT\n' > $@ > + echo '# Constant definitions for the NPTL pretty printers.' >> $@ > + echo '# See gen-py-const.awk for details.\n' >> $@ > + > + # Replace "@name@SOME_NAME@value@SOME_VALUE@" strings from the output of > + # gen-py-const.awk 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 from printers.py 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 will start with a '$'; Make requires that > + # we write '$' twice. these comments are at the shell level instead of makelevel which means they get printed to stdout. would be best to change them to make comments (rather than just adding a @ prefix) so we don't waste time execing a shell to do nothing. > + sed -n 's/^.*@name@\([^@]\+\)@value@\$$\([0-9Xxa-fA-F-]\+\)@.*/\1 = \2/p' \ imo we should use -E rather than doing \( \+ \) stuff so we get POSIX ERE behavior by default also, the use of a-f and such are dangerous. use [:hexdigit:] instead like: [[:hexdigit:]Xx-] > +# Install the Python pretty-printers. > +py-const = $(patsubst %.pysym,%.py,$(gen-py-const-headers)) > +install-bin-script = $(py-const) nptl-printers.py why is this install-bin-script ? we don't want to install these into /usr/bin/. gdb pretty printers usually go into /usr/share/gdb/auto-load/<fspath>. since these are for the pthreads lib, you'd want like: /usr/share/gdb/auto-load/lib/libpthread.so.0 obviously the path & name will vary based on the install of libpthread. we probably also want to add a configure flag for the gdb autoload path as i vaguely recall different distros using different base paths. > +++ b/nptl/nptl-printers.py at a high level, you should run this through pylint > +'info pretty-printer' gdb command. Printers should trigger automatically when GNU style puts two spaces after the period > """ please put before all imports: from __future__ import print_function > +import sys > +import re > +import ctypes > +import gdb system modules should be sorted, then a blank line, then the gdb imports although ctypes looks unused so you should drop it: W: 37, 0: Unused import ctypes (unused-import) > +class _IteratorP3(object): > + """A simple Iterator class.""" this docstring needs to explain what they're for. glancing at their use, i don't see what they're good for. if you need a real iterator, you could just use itertools.chain() and delete these two classes entirely. > +class MutexPrinter(object): glancing at the diff classes, they seem to have ommon behavior. maybe create a local class and implement the common behavior there ? at least the children func looks the same. > +################################################################################ i don't see much value in these lines. just punt them. > + self.values.append(('Threads waiting for this condvar', > + self.nwaiters >> COND_NWAITERS_SHIFT)) trailing indent is wrong: C:376, 0: Wrong continued indentation. self.nwaiters >> COND_NWAITERS_SHIFT)) ^| (bad-continuation) > + def to_string(self): > + """gdb API function. > + > + This is called from gdb when we try to print a pthread_rwlockattr_t.""" > ... > + def children(self): > + """gdb API function. > + > + This is called from gdb when we try to print a pthread_rwlockattr_t.""" both of these docstrings shouldn't be cuddled: C:557, 4: Closing triple quotes should not be cuddled (docstring-cuddled-quotes) C:557, 4: Trailing whitespace in docstring: offset:1: {} (docstring-trailing-whitespace) C:564, 4: Closing triple quotes should not be cuddled (docstring-cuddled-quotes) C:564, 4: Trailing whitespace in docstring: offset:1: {} (docstring-trailing-whitespace) > + def __init__(self, name, regex, callable): is |callable| part of the gdb API ? if not, you should rename it: W:610,40: Redefining built-in 'callable' (redefined-builtin) > + """ > + Initialize a pretty-printer. this should be on the first line: C:610, 8: First line should be a short summary (docstring-first-line) > + def addSubprinter(self, name, regex, callable): same here wrt |callable| > + printer.addSubprinter('pthread_rwlock_t', '^pthread_rwlock_t$', RWLockPrinter) line is too long: C:665, 0: Line too long (82/80) (line-too-long) -mike
Attachment:
signature.asc
Description: Digital signature
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |