Re: [RFC PATCH] seq_file: Use seq_puts when seq_printf has only aformat with no args

[Steven Rostedt]

On Sat, 2013-03-16 at 09:43 -0600, Bjorn Helgaas wrote:

> This is certainly a neat trick.

Thank you ;-)

> 
> But I don't really like the fact that it complicates things for every
> future code reader, especially when a trivial change in the caller
> would accomplish the same thing.  Do you have any idea how much
> performance we would gain in exchange for the complication?

Yeah, it does complicate things a little, but that's what comments are
for. With the comment above the code explaining *exactly* what is
happening, it shouldn't make it difficult for future readers. It may
even inspire them.

It is a trivial change to fix seq_printf("fmt") to seq_puts("fmt"), but
also a maintenance burden. I also find reading "printf" much easier to
read in code than seeing "puts". Imagine:

	seq_printf(m "Format this %s\n", fmt);
	seq_puts(m, "for the following line\n");
	seq_printf(m, "followed by this %d\n", cnt);

It makes it rather ugly to read in the code. Having all users just
default to seq_printf() actually makes the code *easier* to read and to
understand.

Yes, it is complex in one little location that seldom ever changes, and
people seldom even look at. But the result of this change can make it
much more readable throughout the rest of the kernel. When you include
all of the kernel, I think the balance is towards the macro in making
the code easier to read and review. 

I always say, add more complexity in a location that is self contained
and seldom changes, to make things that are all over the place and
changes constantly, less complex.

That was my philosophy for TRACE_EVENT() and my NMI handling. Two very
complex pieces of code, but both self contained, and seldom change.

Also, gcc does this conversion too in userspace. Compile the following
program:

#include <stdio.h>
int main() {
	printf("hello world\n");
	return 0;
}

and do an objdump on it:

  4004c4:       55                      push   %rbp
  4004c5:       48 89 e5                mov    %rsp,%rbp
  4004c8:       48 83 ec 10             sub    $0x10,%rsp
  4004cc:       89 7d fc                mov    %edi,-0x4(%rbp)
  4004cf:       48 89 75 f0             mov    %rsi,-0x10(%rbp)
  4004d3:       bf e8 05 40 00          mov    $0x4005e8,%edi
  4004d8:       e8 db fe ff ff          callq  4003b8 <puts@plt>
  4004dd:       b8 00 00 00 00          mov    $0x0,%eax
  4004e2:       c9                      leaveq 
  4004e3:       c3                      retq   

It converts it to puts!

-- Steve


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/