lkml.org 
[lkml]   [2013]   [Mar]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC PATCH] seq_file: Use seq_puts when seq_printf has only a format with no args
From
Date
On Sat, 2013-03-16 at 09:15 -0700, Joe Perches wrote:

> > > +int (seq_printf)(struct seq_file *m, const char *f, ...)
> >
> > That's rather ugly. Why not just #undef seq_printf before defining it?
>
> The whole thing is ugly, nasty and hackish.
> I kinda like it.
>
> But I don't like unnecessary undefs.
> The preprocessor doesn't expand (funcname).

Either way, as long as it's commented.

>
> > Anyway, not making va_args a whacky name is dangerous. This is why I add
> > those crazy underscores. If someone does:
> >
> > var = 1;
> > va_args[] = "abc";
> > seq_printf(m, "%d %s", var, va_args);
>
> The same could be true of fmt and it's
> used in lots of macros no?

No, not at all. Try it. The difference is that 'fmt' is the macro arg
and gets replaced by the pre-processor, where as va_args is what gets
placed into the C code by the C pre-processor. Any variables that's not
a macro argument MUST BE UNIQUE!

>
> > What will be printed is:
> >
> > 1 var, va_args
> >
> > That will be very confusing to people.
>
> And so be fixed very quickly.

By changing the caller, it's a bug in the implementation of the macro,
not the user of the macro.

>
> > > + if (sizeof(va_args) > 1) \
> > > + seq_printf(seq, fmt, ##__VA_ARGS__); \
> > > + else \
> > > + seq_puts(seq, fmt); \
> > > +} while (0)
> >
> > BTW, you need to return a value.
>
> Oh, yeah, thanks.
>
> > #define seq_printf(seq, fmt, ...) \
> > -do { \
> > +({ \
> > char va_args[] = __stringify(__VA_ARGS__); \
> > + int _____ret; \
> > if (sizeof(va_args) > 1) \
> > - seq_printf(seq, fmt, ##__VA_ARGS__); \
> > + _____ret = seq_printf(seq, fmt, ##__VA_ARGS__); \
> > else \
> > - seq_puts(seq, fmt); \
> > -} while (0)
> > + _____ret = seq_puts(seq, fmt); \
> > + _____ret; \
> > +})
>
> It's certainly better as a statement expression,
> but I think the underscores are really ugly and
> not necessary as ret is locally scoped.

Let me show you the issue:

#define seq_printf(seq, fmt, ...) \
({ \
char va_args[] = __stringify(__VA_ARGS__); \
int ret; \
if (sizeof(va_args) > 1) \
ret = seq_printf(seq, fmt, ##__VA_ARGS__); \
else
ret = seq_puts(seq, fmt); \
ret; \
})

Now lets look at the usage of in the code:

ret = 1;
va_args = 5;
fmt = "hello world\n";
seq_print(m, "my fmt=%s args=%d ret=%d\n", fmt, va_args, ret);

Notice that here we have va_args as a integer.

Here's what cpp does to it (adding newlines to make it readable):

ret = 1;
va_args = 5;
fmt = "hello world";
({
char va_args[] = "fmt, va_args, ret";
int ret;
if (sizeof(va_args) > 1)
ret = seq_printf(seq, "my fmt=%s args=%d ret=%d\n", fmt, va_args, ret);
else
ret = seq_puts(seq, "my fmt=%s args=%d ret=%d\n");
ret;
})

A couple of things here. One, you'll get a gcc warning about ret being
used uninitialized. That should confuse the hell out of the developer.

Then you'll also get a warning about %d expecting a number but the
argument is a string. Another damn confusing thing for developers to
see.

Let me stress it one more time... Any variable names added by a macro,
that's not replaced by the parameters of the macro MUST BE UNIQUE!!!!
Otherwise you will get very difficult hard to debug bugs.

I'm also thinking, as macros may include macros, we may need to define a
macro variable name space policy. Something like:

char __seq_printf__va_args[] = __stringify(__VA_ARGS__);
int __seq_printf__ret;

It may make the macro ugly, but it helps keep conflicts in name spacing.

-- Steve




\
 
 \ /
  Last update: 2013-03-16 18:21    [W:0.040 / U:0.984 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site