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

[Siddhesh Poyarekar <sid at reserved-bit dot com>]

On Sun, Apr 10, 2016 at 05:51:44PM -0300, Martin Galvan wrote:
> Yes, I know that. I've tried to split lines wherever I could, as long
> as it doesn't hurt readability too much. However, doing so on the test
> programs confuses gdb to the point where I need to issue a (somewhat
> random) number of 'next' commands to advance. Those should stay as
> they are.

Please add a comment on top of the tests mentioning that.

> In any case, seeing as there are heaps of code with lines longer than
> 80 chars in the codebase already, and my lines exceed the limit only
> by 4-5 chars at worst, I don't think it's that big an issue.

The codebase has code from a number of different sources, some of
which need to be in sync with the external copies and hence their
formatting differences have been preserved, which is why you may see
differences in formatting.  In any case I don't see that as a valid
reason to introduce more incorrect formatting.  Formatting issues are
never a big deal, but it's useful to keep them as close to the
formatting guidelines to avoid bikeshedding discussions like this one
:)

> >> +$(py-const): $(common-objpfx)%.py: %.pysym $(py-const-script) $(common-before-compile)
> >
> > Line longer than 79 chars.
> 
> I can split it after the target pattern, but it would hurt
> readability. I'd prefer if it stays as it is.

You can put $(common-before-compile) on the next like with a trailing \

> I don't understand. Why isn't it safe?

If your target terminates prematurely due to an error, it may not
resume because the target now already exists.

> I take it that this is what I should do instead. Also, I didn't know
> about that kind of brace expansion until I saw that last line. I guess
> it's the same as 'mv -f $@.tmp2 $@', right?

Yes.

> > Also, you're writing the file to the build root directory.  Since the
> > file will only change occasionally, it would be a good idea to
> > regenerate it only when required and put it in the pretty-printers
> > directory.
> 
> I remember trying to do that and it failing to work. I think it's
> because Makerules runs for each submodule, and the
> build/pretty-printers directory doesn't yet exist when Makerules is
> running for nptl. I can give it another shot, but I don't think it'll
> work.

How about making the file inside pretty-printers and not inside nptl?
Or else you could just make the directory if it doesn't exist yet.

> Yeah, I can look into it after this patch is merged. I think the
> install stuff should be handled first, though.

Agreed.

> >> +we register them for the current objfile by calling gdb.printing.register_pretty_printer().
> >
> > Line longer than 80 chars.
> 
> Does that apply to READMEs as well?

Yes.

> Can't hurt to state it here again :)

Fair enough.

Siddhesh