[PATCH] linux/string.h: Introduce streq macro.

[PATCH] linux/string.h: Introduce streq macro.

[Thiago Farina]

This macro is arguably more readable than its variants:
- !strcmp(a, b)
- strcmp(a, b) == 0

Signed-off-by: Thiago Farina <tfransosi@gmail.com>
---
 include/linux/string.h |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/include/linux/string.h b/include/linux/string.h
index a716ee2..15b9602 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -134,6 +134,17 @@ extern ssize_t memory_read_from_buffer(void *to, size_t count, loff_t *ppos,
 			const void *from, size_t available);
 
 /**
+ * streq - Are two strings equal?
+ * @a: first string
+ * @b: second string
+ *
+ * Example:
+ *	if (streq(argv[1], "--help"))
+ *		printf("%s\n", "This help");
+ */
+#define streq(a, b) (strcmp((a), (b)) == 0)
+
+/**
  * strstarts - does @str start with @prefix?
  * @str: string to examine
  * @prefix: prefix to look for.
-- 
1.7.5.rc2.5.g60e19

Re: [PATCH] linux/string.h: Introduce streq macro.

[Steven Rostedt]

On Tue, 2011-04-26 at 15:49 -0300, Thiago Farina wrote:
> This macro is arguably more readable than its variants:
> - !strcmp(a, b)
> - strcmp(a, b) == 0
> 

If this is acceptable, perhaps we should push this for cleanups around
the kernel.

> Signed-off-by: Thiago Farina <tfransosi@gmail.com>

Acked-by: Steven Rostedt <rostedt@goodmis.org>

> ---
>  include/linux/string.h |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/string.h b/include/linux/string.h
> index a716ee2..15b9602 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -134,6 +134,17 @@ extern ssize_t memory_read_from_buffer(void *to, size_t count, loff_t *ppos,
>  			const void *from, size_t available);
>  
>  /**
> + * streq - Are two strings equal?
> + * @a: first string
> + * @b: second string
> + *
> + * Example:
> + *	if (streq(argv[1], "--help"))
> + *		printf("%s\n", "This help");
> + */
> +#define streq(a, b) (strcmp((a), (b)) == 0)

Are the extra parenthesis around the 'a' and 'b' necessary? Anything
used in streq() would be the same as what is in strcmp(). Actually, this
may be even better to make it a static inline.

static inline int streq(const char *a, const char *b)
{
	return strcmp(a, b) == 0;
}


-- Steve

> +
> +/**
>   * strstarts - does @str start with @prefix?
>   * @str: string to examine
>   * @prefix: prefix to look for.

Re: [PATCH] linux/string.h: Introduce streq macro.

[Alexey Dobriyan]

On Tue, Apr 26, 2011 at 03:49:49PM -0300, Thiago Farina wrote:
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -134,6 +134,17 @@ extern ssize_t memory_read_from_buffer(void *to, size_t count, loff_t *ppos,
>  			const void *from, size_t available);
>  
>  /**
> + * streq - Are two strings equal?
> + * @a: first string
> + * @b: second string
> + *
> + * Example:
> + *	if (streq(argv[1], "--help"))
> + *		printf("%s\n", "This help");
> + */

Oh, come on!
Next, you're going to explain if statement.

> +#define streq(a, b) (strcmp((a), (b)) == 0)

It should be inline function.

Re: [PATCH] linux/string.h: Introduce streq macro.

[Steven Rostedt]

On Tue, 2011-04-26 at 22:05 +0300, Alexey Dobriyan wrote:
> On Tue, Apr 26, 2011 at 03:49:49PM -0300, Thiago Farina wrote:
> > --- a/include/linux/string.h
> > +++ b/include/linux/string.h
> > @@ -134,6 +134,17 @@ extern ssize_t memory_read_from_buffer(void *to, size_t count, loff_t *ppos,
> >  			const void *from, size_t available);
> >  
> >  /**
> > + * streq - Are two strings equal?
> > + * @a: first string
> > + * @b: second string
> > + *
> > + * Example:
> > + *	if (streq(argv[1], "--help"))
> > + *		printf("%s\n", "This help");

Userspace example?

> > + */
> 
> Oh, come on!
> Next, you're going to explain if statement.

I already have: include/linux/compiler.h

/*
 * "Define 'is'", Bill Clinton
 * "Define 'if'", Steven Rostedt
 */
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
#define __trace_if(cond) \
	if (__builtin_constant_p((cond)) ? !!(cond) :			\
	({								\
		int ______r;						\
		static struct ftrace_branch_data			\
			__attribute__((__aligned__(4)))			\
			__attribute__((section("_ftrace_branch")))	\
			______f = {					\
				.func = __func__,			\
				.file = __FILE__,			\
				.line = __LINE__,			\
			};						\
		______r = !!(cond);					\
		______f.miss_hit[______r]++;					\
		______r;						\
	}))

;)

-- Steve

> 
> > +#define streq(a, b) (strcmp((a), (b)) == 0)
> 
> It should be inline function.

Re: [PATCH] linux/string.h: Introduce streq macro.

[Alexey Dobriyan]

On Tue, Apr 26, 2011 at 03:17:48PM -0400, Steven Rostedt wrote:
> On Tue, 2011-04-26 at 22:05 +0300, Alexey Dobriyan wrote:
> > On Tue, Apr 26, 2011 at 03:49:49PM -0300, Thiago Farina wrote:

> > >  /**
> > > + * streq - Are two strings equal?
> > > + * @a: first string
> > > + * @b: second string
> > > + *
> > > + * Example:
> > > + *	if (streq(argv[1], "--help"))
> > > + *		printf("%s\n", "This help");
> 
> Userspace example?

The point is that function is trivial, and if someone doesn't
understand it, he should read some Kernighan and Ritchie first.

Re: [PATCH] linux/string.h: Introduce streq macro.

[Thiago Farina]

On Tue, Apr 26, 2011 at 4:20 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> On Tue, Apr 26, 2011 at 03:17:48PM -0400, Steven Rostedt wrote:
>> On Tue, 2011-04-26 at 22:05 +0300, Alexey Dobriyan wrote:
>> > On Tue, Apr 26, 2011 at 03:49:49PM -0300, Thiago Farina wrote:
>
>> > >  /**
>> > > + * streq - Are two strings equal?
>> > > + * @a: first string
>> > > + * @b: second string
>> > > + *
>> > > + * Example:
>> > > + *       if (streq(argv[1], "--help"))
>> > > + *               printf("%s\n", "This help");
>>
>> Userspace example?
>
> The point is that function is trivial, and if someone doesn't
> understand it, he should read some Kernighan and Ritchie first.
>

I'm fine to remove (should I Steven?). I don't care about the example.
People reading the code here knows what is this for.

Re: [PATCH] linux/string.h: Introduce streq macro.

[Steven Rostedt]

On Tue, 2011-04-26 at 22:20 +0300, Alexey Dobriyan wrote:
> On Tue, Apr 26, 2011 at 03:17:48PM -0400, Steven Rostedt wrote:
> > On Tue, 2011-04-26 at 22:05 +0300, Alexey Dobriyan wrote:
> > > On Tue, Apr 26, 2011 at 03:49:49PM -0300, Thiago Farina wrote:
> 
> > > >  /**
> > > > + * streq - Are two strings equal?
> > > > + * @a: first string
> > > > + * @b: second string
> > > > + *
> > > > + * Example:
> > > > + *	if (streq(argv[1], "--help"))
> > > > + *		printf("%s\n", "This help");
> > 
> > Userspace example?
> 
> The point is that function is trivial, and if someone doesn't
> understand it, he should read some Kernighan and Ritchie first.

I understood your point, but I missed looking at it in my first reply.
I'm just stating that it is even worse since it includes a userspace
example of a macro defined in the kernel. IOW, the example wont even
work.

-- Steve

Re: [PATCH] linux/string.h: Introduce streq macro.

[Steven Rostedt]

On Tue, 2011-04-26 at 16:21 -0300, Thiago Farina wrote:
> On Tue, Apr 26, 2011 at 4:20 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > On Tue, Apr 26, 2011 at 03:17:48PM -0400, Steven Rostedt wrote:
> >> On Tue, 2011-04-26 at 22:05 +0300, Alexey Dobriyan wrote:
> >> > On Tue, Apr 26, 2011 at 03:49:49PM -0300, Thiago Farina wrote:
> >
> >> > >  /**
> >> > > + * streq - Are two strings equal?
> >> > > + * @a: first string
> >> > > + * @b: second string
> >> > > + *
> >> > > + * Example:
> >> > > + *       if (streq(argv[1], "--help"))
> >> > > + *               printf("%s\n", "This help");
> >>
> >> Userspace example?
> >
> > The point is that function is trivial, and if someone doesn't
> > understand it, he should read some Kernighan and Ritchie first.
> >
> 
> I'm fine to remove (should I Steven?). I don't care about the example.
> People reading the code here knows what is this for.

Replace it with something like:

	* Use this: 	streq(a, b)
	* instead of: 	strcmp(a, b) == 0 or !strcmp(a, b)
	* 
	* This makes the code more readable and less error prone.

-- Steve

Re: [PATCH] linux/string.h: Introduce streq macro.

[Joe Perches]

On Tue, 2011-04-26 at 15:37 -0400, Steven Rostedt wrote:
> On Tue, 2011-04-26 at 16:21 -0300, Thiago Farina wrote:
> > On Tue, Apr 26, 2011 at 4:20 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > > On Tue, Apr 26, 2011 at 03:17:48PM -0400, Steven Rostedt wrote:
> > >> On Tue, 2011-04-26 at 22:05 +0300, Alexey Dobriyan wrote:
> > >> > On Tue, Apr 26, 2011 at 03:49:49PM -0300, Thiago Farina wrote:
> > >> > > + * streq - Are two strings equal?
> > > The point is that function is trivial, and if someone doesn't
> > > understand it, he should read some Kernighan and Ritchie first.
> Replace it with something like:
> 	* Use this: 	streq(a, b)
> 	* instead of: 	strcmp(a, b) == 0 or !strcmp(a, b)
> 	* This makes the code more readable and less error prone.

I think it's not good to introduce another form.
strcmp is the standard everyone understands.

There are already about 2800 uses of strcmp()==0 and !strcmp

$ grep -rP --include=*.[ch] "\bstrcmp.*==\s*0\b" * | wc -l
1143
$ grep -rP --include=*.[ch] "\!\s*strcmp\s*\(" *  | wc -l
1663

Can you count how many misuses of strcmp have been
corrected?  Do you plan to convert the existing 2800?

[PATCH] linux/string.h: Introduce streq macro.

[Thiago Farina]

This macro is arguably more readable than its variants:
- !strcmp(a, b)
- strcmp(a, b) == 0

Signed-off-by: Thiago Farina <tfransosi@gmail.com>
---
 Changes from v1 (Steven and Alexey review):
 - Convert from macro to static inline.
 - Remove the example.
 - Add the suggested comment by Steven.

 include/linux/string.h |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/include/linux/string.h b/include/linux/string.h
index a716ee2..d859bb2 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -134,6 +134,21 @@ extern ssize_t memory_read_from_buffer(void *to, size_t count, loff_t *ppos,
 			const void *from, size_t available);
 
 /**
+ * streq - Are two strings equal?
+ * @a: first string
+ * @b: second string
+ *
+ * Use: streq(a, b)
+ * Instead of: strcmp(a, b) == 0 or !strcmp(a, b)
+ *
+ * This makes the code more readable and less error prone.
+ */
+static inline int streq(const char *a, const char *b)
+{
+	return strcmp(a, b) == 0;
+}
+
+/**
  * strstarts - does @str start with @prefix?
  * @str: string to examine
  * @prefix: prefix to look for.
-- 
1.7.5.rc2.5.g60e19

Re: [PATCH] linux/string.h: Introduce streq macro.

[Thiago Farina]

On Tue, Apr 26, 2011 at 4:45 PM, Joe Perches <joe@perches.com> wrote:
> On Tue, 2011-04-26 at 15:37 -0400, Steven Rostedt wrote:
>> On Tue, 2011-04-26 at 16:21 -0300, Thiago Farina wrote:
>> > On Tue, Apr 26, 2011 at 4:20 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
>> > > On Tue, Apr 26, 2011 at 03:17:48PM -0400, Steven Rostedt wrote:
>> > >> On Tue, 2011-04-26 at 22:05 +0300, Alexey Dobriyan wrote:
>> > >> > On Tue, Apr 26, 2011 at 03:49:49PM -0300, Thiago Farina wrote:
>> > >> > > + * streq - Are two strings equal?
>> > > The point is that function is trivial, and if someone doesn't
>> > > understand it, he should read some Kernighan and Ritchie first.
>> Replace it with something like:
>>       * Use this:     streq(a, b)
>>       * instead of:   strcmp(a, b) == 0 or !strcmp(a, b)
>>       * This makes the code more readable and less error prone.
>
> I think it's not good to introduce another form.
> strcmp is the standard everyone understands.
>
> There are already about 2800 uses of strcmp()==0 and !strcmp
>
> $ grep -rP --include=*.[ch] "\bstrcmp.*==\s*0\b" * | wc -l
> 1143
> $ grep -rP --include=*.[ch] "\!\s*strcmp\s*\(" *  | wc -l
> 1663
>
> Can you count how many misuses of strcmp have been
> corrected?

> Do you plan to convert the existing 2800?

I'd work on this without any problem.

Re: [PATCH] linux/string.h: Introduce streq macro.

[Thiago Farina]

+Copying Andrew and Joe.

On Tue, Apr 26, 2011 at 4:45 PM, Thiago Farina <tfransosi@gmail.com> wrote:
> This macro is arguably more readable than its variants:
> - !strcmp(a, b)
> - strcmp(a, b) == 0
>
> Signed-off-by: Thiago Farina <tfransosi@gmail.com>
> ---
>  Changes from v1 (Steven and Alexey review):
>  - Convert from macro to static inline.
>  - Remove the example.
>  - Add the suggested comment by Steven.
>
>  include/linux/string.h |   15 +++++++++++++++
>  1 files changed, 15 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index a716ee2..d859bb2 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -134,6 +134,21 @@ extern ssize_t memory_read_from_buffer(void *to, size_t count, loff_t *ppos,
>                        const void *from, size_t available);
>
>  /**
> + * streq - Are two strings equal?
> + * @a: first string
> + * @b: second string
> + *
> + * Use: streq(a, b)
> + * Instead of: strcmp(a, b) == 0 or !strcmp(a, b)
> + *
> + * This makes the code more readable and less error prone.
> + */
> +static inline int streq(const char *a, const char *b)
> +{
> +       return strcmp(a, b) == 0;
> +}
> +
> +/**
>  * strstarts - does @str start with @prefix?
>  * @str: string to examine
>  * @prefix: prefix to look for.
> --
> 1.7.5.rc2.5.g60e19
>
>

Re: [PATCH] linux/string.h: Introduce streq macro.

[Steven Rostedt]

On Tue, 2011-04-26 at 16:47 -0300, Thiago Farina wrote:

> > $ grep -rP --include=*.[ch] "\bstrcmp.*==\s*0\b" * | wc -l
> > 1143
> > $ grep -rP --include=*.[ch] "\!\s*strcmp\s*\(" *  | wc -l
> > 1663
> >
> > Can you count how many misuses of strcmp have been
> > corrected?
> 
> > Do you plan to convert the existing 2800?
> 
> I'd work on this without any problem.

Nothing a perl script can't do either.

-- Steve

Re: [PATCH] linux/string.h: Introduce streq macro.

[Steven Rostedt]

On Tue, 2011-04-26 at 16:47 -0300, Thiago Farina wrote:

> > Do you plan to convert the existing 2800?
> 
> I'd work on this without any problem.

Oh, and make sure you do each change as a separate patch to guarantee
that you get to the top of the "most contributed to the Linux kernel" on
LWN's who's done what article ;)

-- Steve

Re: [PATCH] linux/string.h: Introduce streq macro.

[Joe Perches]

On Tue, 2011-04-26 at 15:58 -0400, Steven Rostedt wrote:
> On Tue, 2011-04-26 at 16:47 -0300, Thiago Farina wrote:
> > > $ grep -rP --include=*.[ch] "\bstrcmp.*==\s*0\b" * | wc -l
> > > 1143
> > > $ grep -rP --include=*.[ch] "\!\s*strcmp\s*\(" *  | wc -l
> > > 1663
> > > Can you count how many misuses of strcmp have been
> > > corrected?
> > > Do you plan to convert the existing 2800?
> > I'd work on this without any problem.
> Nothing a perl script can't do either.

That's not a real problem.

my $match_balanced_parentheses = qr/(\((?:[^\(\)]++|(?-1))*\))/;
s/\bstrcmp\s*${match_balanced_parentheses}\s*==\s*0\b/streq$1/g;
s/\!\s*strcmp\s*\(/streq\(/g;

And the other strcmp uses too?

Re: [PATCH] linux/string.h: Introduce streq macro.

[Davidlohr Bueso]

On Tue, 2011-04-26 at 15:49 -0300, Thiago Farina wrote:
> This macro is arguably more readable than its variants:
> - !strcmp(a, b)
> - strcmp(a, b) == 0

Anyone how is going to be reading kernel C code, can understand strcmp I
think.

Cheers,
Davidlohr

Re: [PATCH] linux/string.h: Introduce streq macro.

[Thiago Farina]

On Tue, Apr 26, 2011 at 5:27 PM, Davidlohr Bueso <dave@gnu.org> wrote:
> On Tue, 2011-04-26 at 15:49 -0300, Thiago Farina wrote:
>> This macro is arguably more readable than its variants:
>> - !strcmp(a, b)
>> - strcmp(a, b) == 0
>
> Anyone how is going to be reading kernel C code, can understand strcmp I
> think.
>
We all know that, but please see the previous discussion.

http://www.spinics.net/lists/kernel/msg1176975.html
http://www.spinics.net/lists/kernel/msg1176977.html

Best regards,

> Cheers,
> Davidlohr
>
>

Re: [PATCH] linux/string.h: Introduce streq macro.

[Ted Ts'o]

On Tue, Apr 26, 2011 at 03:49:49PM -0300, Thiago Farina wrote:
> This macro is arguably more readable than its variants:
> - !strcmp(a, b)
> - strcmp(a, b) == 0
> 
> Signed-off-by: Thiago Farina <tfransosi@gmail.com>

I don't think this is not a good idea.

First of all, changing 2800 instances of strcmp will induce a huge
amount of code churn, that will cause patches to break, etc.  And
whether streq() looks better is going to be very much a case of
personal preference.  I'm so used to !strcmp(a, b) that streq(a, b)
would be harder for me, just because I'm not used to it.

So I'd NACK a change like this to any parts of the kernel that I'm
maintaining.  If another people feel that way, it's not clear that
having two different conventions in the kernel would necessarily help...

       	   	     		    	- Ted

Re: [PATCH] linux/string.h: Introduce streq macro.

[Steven Rostedt]

On Tue, 2011-04-26 at 20:52 -0400, Ted Ts'o wrote:
> On Tue, Apr 26, 2011 at 03:49:49PM -0300, Thiago Farina wrote:
> > This macro is arguably more readable than its variants:
> > - !strcmp(a, b)
> > - strcmp(a, b) == 0
> > 
> > Signed-off-by: Thiago Farina <tfransosi@gmail.com>
> 
> I don't think this is not a good idea.
> 
> First of all, changing 2800 instances of strcmp will induce a huge
> amount of code churn, that will cause patches to break, etc.  And
> whether streq() looks better is going to be very much a case of
> personal preference.  I'm so used to !strcmp(a, b) that streq(a, b)
> would be harder for me, just because I'm not used to it.
> 
> So I'd NACK a change like this to any parts of the kernel that I'm
> maintaining.  If another people feel that way, it's not clear that
> having two different conventions in the kernel would necessarily help...
> 

I agree that this entire thing is all about personal preference, and
that a lot of these conventions is determined by who maintains the code.

This all started when I changed code that I need to maintain from:

	if (0 == var)

to

	if (var == 0)

I understand why the first is done, but it just trips me up every time I
see it.

I also prefer:

	if (strcmp(a, b) == 0)

over

	if (!strcmp(a, b))

because the ! in that statement makes my mind say "a != b". But again,
this is all about preference.

As for me, I would not mind a streq() as it is a different
function/macro, that I would not get it confused with strcmp().

I acked the patch, because I would not NAK changes that converted the
strcmp() to streq() in my code (as long as it was done correctly).

-- Steve

Re: [PATCH] linux/string.h: Introduce streq macro.

[Christoph Hellwig]

On Tue, Apr 26, 2011 at 08:52:43PM -0400, Ted Ts'o wrote:
> I don't think this is not a good idea.
> 
> First of all, changing 2800 instances of strcmp will induce a huge
> amount of code churn, that will cause patches to break, etc.  And
> whether streq() looks better is going to be very much a case of
> personal preference.  I'm so used to !strcmp(a, b) that streq(a, b)
> would be harder for me, just because I'm not used to it.
> 
> So I'd NACK a change like this to any parts of the kernel that I'm
> maintaining.  If another people feel that way, it's not clear that
> having two different conventions in the kernel would necessarily help...

Same here.  Diverging from standard ANSI C just for the sake of being
different is an utterly bad idea.  strcmp might not be the most natural
calling convention, but it's been in the wild for 30 years, and everyone
taking a C 101 course should know about it.

And if you get it wrong and don't notice it just means your testing
coverage sucks badly.

Re: [PATCH] linux/string.h: Introduce streq macro.

[Miguel Ojeda]

On Tue, Apr 26, 2011 at 9:58 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Tue, 2011-04-26 at 16:47 -0300, Thiago Farina wrote:
>
>> > $ grep -rP --include=*.[ch] "\bstrcmp.*==\s*0\b" * | wc -l
>> > 1143
>> > $ grep -rP --include=*.[ch] "\!\s*strcmp\s*\(" *  | wc -l
>> > 1663
>> >
>> > Can you count how many misuses of strcmp have been
>> > corrected?
>>
>> > Do you plan to convert the existing 2800?
>>
>> I'd work on this without any problem.
>
> Nothing a perl script can't do either.

If you are really going to do that, please use a coccinelle's semantic
patch (which is designed precisely for that purpose) and document it
so that it can be included in the standard set of semantic patches
applied to the kernel regularly, i.e. something like:

@@ expression E1, E2; @@
- strcmp(E1,E2) == 0
+ streq(E1,E2)

@@ expression E1, E2; @@
- strcmp(E1,E2) != 0
+ !streq(E1,E2)

@@ expression E1, E2; @@
- !strcmp(E1,E2)
+ streq(E1,E2)

@@ expression E1, E2; @@
- strcmp(E1,E2)
+ !streq(E1,E2)

This produces a patch ~1.1 MB in size, which is probably going to
produce some headaches when merging other patches...

$ diffstat streq.patch | tail -1
 1073 files changed, 3429 insertions(+), 3419 deletions(-)

$ diffstat streq.patch | sort -nk 3 | tail -20
 drivers/sbus/char/envctrl.c                                     |   30 -
 fs/reiserfs/bitmap.c                                            |   30 -
 sound/pci/hda/hda_eld.c                                         |   30 -
 drivers/video/matrox/matroxfb_base.c                            |   32 -
 arch/blackfin/kernel/module.c                                   |   34 +-
 tools/perf/util/symbol.c                                        |   35 +-
 scripts/mod/modpost.c                                           |   38 +-
 net/irda/irlan/irlan_client.c                                   |   40 +-
 drivers/staging/hv/vmbus_drv.c                                  |   42 +-
 drivers/usb/gadget/gadget_chips.h                               |   42 +-
 drivers/s390/net/qeth_l3_sys.c                                  |   44 +-
 drivers/macintosh/windfarm_smu_controls.c                       |   48 +-
 security/selinux/hooks.c                                        |   48 +-
 security/smack/smack_lsm.c                                      |   48 +-
 arch/x86/pci/common.c                                           |   50 +--
 drivers/net/niu.c                                               |   54 +--
 fs/xfs/linux-2.6/xfs_super.c                                    |   92 ++---
 tools/perf/util/trace-event-parse.c                             |  108 +++---
 drivers/staging/wlags49_h2/wl_profile.c                         |  121 +++----
 net/core/pktgen.c                                               |
164 +++++-----

Miguel Ojeda

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

Re: [PATCH] linux/string.h: Introduce streq macro.

[Mike Frysinger]

On Tue, Apr 26, 2011 at 14:49, Thiago Farina wrote:
> This macro is arguably more readable than its variants:
> - !strcmp(a, b)
> - strcmp(a, b) == 0

Acked-by: Mike Frysinger <vapier@gentoo.org>
(i dont mind converting any of the Blackfin code to use this)
-mike

Re: [PATCH] linux/string.h: Introduce streq macro.

[Geert Uytterhoeven]

On Wed, Apr 27, 2011 at 10:29, Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
> On Tue, Apr 26, 2011 at 9:58 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>> On Tue, 2011-04-26 at 16:47 -0300, Thiago Farina wrote:
>>
>>> > $ grep -rP --include=*.[ch] "\bstrcmp.*==\s*0\b" * | wc -l
>>> > 1143
>>> > $ grep -rP --include=*.[ch] "\!\s*strcmp\s*\(" *  | wc -l
>>> > 1663
>>> >
>>> > Can you count how many misuses of strcmp have been
>>> > corrected?
>>>
>>> > Do you plan to convert the existing 2800?
>>>
>>> I'd work on this without any problem.
>>
>> Nothing a perl script can't do either.
>
> If you are really going to do that, please use a coccinelle's semantic
> patch (which is designed precisely for that purpose) and document it
> so that it can be included in the standard set of semantic patches
> applied to the kernel regularly, i.e. something like:

First, I agree (mildly) with the opponents: strcmp() is standard C99.
Second, as the goal is to avoid bugs, this conversion should not be blindly
done by a script and be done with it, but reviewed by a human.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] linux/string.h: Introduce streq macro.

[gmack]

On Wed, 27 Apr 2011, Christoph Hellwig wrote:

> Date: Wed, 27 Apr 2011 02:47:19 -0400
> From: Christoph Hellwig <hch@infradead.org>
> To: Ted Ts'o <tytso@mit.edu>, Thiago Farina <tfransosi@gmail.com>,
>     linux-kernel@vger.kernel.org, Steven Rostedt <rostedt@goodmis.org>
> Subject: Re: [PATCH] linux/string.h: Introduce streq macro.
> 
> On Tue, Apr 26, 2011 at 08:52:43PM -0400, Ted Ts'o wrote:
> > I don't think this is not a good idea.
> > 
> > First of all, changing 2800 instances of strcmp will induce a huge
> > amount of code churn, that will cause patches to break, etc.  And
> > whether streq() looks better is going to be very much a case of
> > personal preference.  I'm so used to !strcmp(a, b) that streq(a, b)
> > would be harder for me, just because I'm not used to it.
> > 
> > So I'd NACK a change like this to any parts of the kernel that I'm
> > maintaining.  If another people feel that way, it's not clear that
> > having two different conventions in the kernel would necessarily help...
> 
> Same here.  Diverging from standard ANSI C just for the sake of being
> different is an utterly bad idea.  strcmp might not be the most natural
> calling convention, but it's been in the wild for 30 years, and everyone
> taking a C 101 course should know about it.
> 
> And if you get it wrong and don't notice it just means your testing
> coverage sucks badly.

Knowing about it and not screwing it up are two different things. I was 
working on a project a few years ago and we made this exact change thanks 
to the backwards logic of strcmp constantly screwing people up and the bug 
count went down considerably.


	Gerhard


--
Gerhard Mack

gmack@innerfire.net

<>< As a computer I find your faith in technology amusing.

Re: [PATCH] linux/string.h: Introduce streq macro.

[Miguel Ojeda]

On Wed, Apr 27, 2011 at 10:42 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Wed, Apr 27, 2011 at 10:29, Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
>> On Tue, Apr 26, 2011 at 9:58 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>>> On Tue, 2011-04-26 at 16:47 -0300, Thiago Farina wrote:
>>>
>>>> > $ grep -rP --include=*.[ch] "\bstrcmp.*==\s*0\b" * | wc -l
>>>> > 1143
>>>> > $ grep -rP --include=*.[ch] "\!\s*strcmp\s*\(" *  | wc -l
>>>> > 1663
>>>> >
>>>> > Can you count how many misuses of strcmp have been
>>>> > corrected?
>>>>
>>>> > Do you plan to convert the existing 2800?
>>>>
>>>> I'd work on this without any problem.
>>>
>>> Nothing a perl script can't do either.
>>
>> If you are really going to do that, please use a coccinelle's semantic
>> patch (which is designed precisely for that purpose) and document it
>> so that it can be included in the standard set of semantic patches
>> applied to the kernel regularly, i.e. something like:
>
> First, I agree (mildly) with the opponents: strcmp() is standard C99.
> Second, as the goal is to avoid bugs, this conversion should not be blindly
> done by a script and be done with it, but reviewed by a human.

Hi, on the contrary: I don't agree with the change unless a policy for
all this kind of changes (i.e. changes which diverge from C99) is
written and discussed first for all the kernel (and not only in
strcmp's regard). I was just warning about the potential size of the
patch (like Ted Ts'o did) and the dangers of doing it with a quickly
hacked perl script. Using a coccinelle semantic patch would much
better suited for this and less error prone if this kind of changes
are ultimately merged.

Regards,
Miguel Ojeda

>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
>

Re: [PATCH] linux/string.h: Introduce streq macro.

[Pavel Vasilyev]

[-- Attachment #1: Type: text/plain, Size: 1225 bytes --]

27.04.2011 12:49, Miguel Ojeda пишет:
> On Wed, Apr 27, 2011 at 10:42 AM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Wed, Apr 27, 2011 at 10:29, Miguel Ojeda
>> <miguel.ojeda.sandonis@gmail.com> wrote:
>>> On Tue, Apr 26, 2011 at 9:58 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>>>> On Tue, 2011-04-26 at 16:47 -0300, Thiago Farina wrote:
>>>>
>>>>>> $ grep -rP --include=*.[ch] "\bstrcmp.*==\s*0\b" * | wc -l
>>>>>> 1143
>>>>>> $ grep -rP --include=*.[ch] "\!\s*strcmp\s*\(" *  | wc -l
>>>>>> 1663
>>>>>>
>>>>>> Can you count how many misuses of strcmp have been
>>>>>> corrected?
>>>>>
>>>>>> Do you plan to convert the existing 2800?
>>>>>
>>>>> I'd work on this without any problem.
>>>>
>>>> Nothing a perl script can't do either.
>>>
>>> If you are really going to do that, please use a coccinelle's semantic
>>> patch (which is designed precisely for that purpose) and document it

> Hi, on the contrary: I don't agree with the change unless a policy ...

I came up with the best :)

# cd /usr/src/linux-2.6

# for i in `grep strncmp ./ -R | cut -d: -f1,1`; \
    do \
       sed -i '/strncmp/ s//memcmp/g;' $i; \
done;





-- 

                                                         Pavel.

[-- Attachment #2: 0x03742489.asc --]
[-- Type: application/pgp-keys, Size: 2524 bytes --]

Re: [PATCH] linux/string.h: Introduce streq macro.

[Ted Ts'o]

On Wed, Apr 27, 2011 at 04:47:40AM -0400, gmack@innerfire.net wrote:
> Knowing about it and not screwing it up are two different things. I was 
> working on a project a few years ago and we made this exact change thanks 
> to the backwards logic of strcmp constantly screwing people up and the bug 
> count went down considerably.

If someone could even vaguely possibly screw up strcmp(), I don't want
them submitting patches to my subsystem.  I'm generally worried about
far more subtle bugs (deadlocks, locking screwups), and as Christoph
said, if you can't notice a strcmp bug, there's something
***seriousl**** wrong with your code review process, test suite,
testing discpline, or all of the above.

I would consider patches to change !strcmp() to streq() in any code I
maintain to be worse noise than spelling patches, or whitespace
patches.

						- Ted

Re: [PATCH] linux/string.h: Introduce streq macro.

[Alexey Dobriyan]

On Wed, Apr 27, 2011 at 5:52 PM, Ted Ts'o <tytso@mit.edu> wrote:
> On Wed, Apr 27, 2011 at 04:47:40AM -0400, gmack@innerfire.net wrote:
>> Knowing about it and not screwing it up are two different things. I was
>> working on a project a few years ago and we made this exact change thanks
>> to the backwards logic of strcmp constantly screwing people up and the bug
>> count went down considerably.
>
> If someone could even vaguely possibly screw up strcmp(), I don't want
> them submitting patches to my subsystem.  I'm generally worried about
> far more subtle bugs (deadlocks, locking screwups), and as Christoph
> said, if you can't notice a strcmp bug, there's something
> ***seriousl**** wrong with your code review process, test suite,
> testing discpline, or all of the above.

This can be good excuse in math where proof of existence is enough.
For some reason, the one making excuses always fails to say
what exactly is wrong with all of the above.

> I would consider patches to change !strcmp() to streq() in any code I
> maintain to be worse noise than spelling patches, or whitespace
> patches.

If someone finds strcmp bug in e2fsprogs or ext[234] code,
will you change your opinion?


People are trying to add "string comparison by value" idiom
the way it's supposed to be.
Adding it into C2042 is very hard, so they add it into kernel.

But instead of agreeing
that, yes, streq() as "compare strings by value, relative order is not
important,
interesting or relevant" should have existed from the very beginning,
let's add it into kernel at least,

that, yes, strcmp() usage bugs happened
(side note: kernel doesn't have in-tree testsuite, only chaotic tests,
so what do these kernel guys know about testing their own code?)

that, yes, 3 ways to write "S1 equals S2" sucks.

that, yes, penny-wise Unix is in the past,

instead, there is usual ignorant hand-waving.

P. S.: I for one glad kmemdup() is in tree.

Re: [PATCH] linux/string.h: Introduce streq macro.

[Steven Rostedt]

On Wed, 2011-04-27 at 19:04 +0300, Alexey Dobriyan wrote:
> On Wed, Apr 27, 2011 at 5:52 PM, Ted Ts'o <tytso@mit.edu> wrote:
> > On Wed, Apr 27, 2011 at 04:47:40AM -0400, gmack@innerfire.net wrote:
> >> Knowing about it and not screwing it up are two different things. I was
> >> working on a project a few years ago and we made this exact change thanks
> >> to the backwards logic of strcmp constantly screwing people up and the bug
> >> count went down considerably.
> >
> > If someone could even vaguely possibly screw up strcmp(), I don't want
> > them submitting patches to my subsystem.  I'm generally worried about
> > far more subtle bugs (deadlocks, locking screwups), and as Christoph
> > said, if you can't notice a strcmp bug, there's something
> > ***seriousl**** wrong with your code review process, test suite,
> > testing discpline, or all of the above.
> 
> This can be good excuse in math where proof of existence is enough.
> For some reason, the one making excuses always fails to say
> what exactly is wrong with all of the above.

Note, Ted only NAK'd it for his own code. Christoph seemed to do the
same for his. I'm fine with it in my code as when I see:

	if (le32_to_cpu(de->inode) != inode->i_ino ||
			!le32_to_cpu(de1->inode) ||
			strcmp (".", de->name) ||
			strcmp ("..", de1->name)) {
		ext3_warning (inode->i_sb, "empty_dir",
			      "bad directory (dir #%lu) - no `.' or `..'",
			      inode->i_ino);
		brelse (bh);
		return 1;
	}

(from fs/ext3/namei.c)

it does not stand out to me that this if statement will return true if
de->name does not equal "." or de1->name does not equal "..". If this
looked like:

	if (le32_to_cpu(de->inode) != inode->i_ino ||
			!le32_to_cpu(de1->inode) ||
			!streq (".", de->name) ||
			!streq ("..", de1->name)) {
		ext3_warning (inode->i_sb, "empty_dir",
			      "bad directory (dir #%lu) - no `.' or `..'",
			      inode->i_ino);
		brelse (bh);
		return 1;
	}

IMO looks much easier to understand.

Although I also think this is easier to understand too:

	if (le32_to_cpu(de->inode) != inode->i_ino ||
			!le32_to_cpu(de1->inode) ||
			strcmp (".", de->name) != 0 ||
			strcmp ("..", de1->name) != 0) {
		ext3_warning (inode->i_sb, "empty_dir",
			      "bad directory (dir #%lu) - no `.' or `..'",
			      inode->i_ino);
		brelse (bh);
		return 1;
	}


But again, I'm not the maintainer that needs to make sure this is
correct. I still feel this is personal preference decided by the
maintainer of said code.


> But instead of agreeing
> that, yes, streq() as "compare strings by value, relative order is not
> important,
> interesting or relevant" should have existed from the very beginning,
> let's add it into kernel at least,

I'm all for adding a streq() and may even use it. Simply because it
makes reading the code easier to understand, and not necessarily less
buggy (although I think it might help).

Note, I was having a similar discussion with a fellow kernel developer
on IRC a few years ago about using:

	!strcmp() vs strcmp() == 0

and in that discussion, the example the other developer had done with 
!strcmp() had a bug in it. Which did convince him to switch to
strcmp() == 0 ;)

-- Steve

Re: [PATCH] linux/string.h: Introduce streq macro.

[Al Viro]

On Tue, Apr 26, 2011 at 03:49:49PM -0300, Thiago Farina wrote:
> This macro is arguably more readable than its variants:
> - !strcmp(a, b)
> - strcmp(a, b) == 0

Strongly NACKed.  As far as I'm concerned, it's in the same shitbucket as
bcopy(3), bzero(3) et.al.  Use idiomatic C; extensions of that kind are
*bad*, since new developers have to learn them.

Re: [PATCH] linux/string.h: Introduce streq macro.

[Steven Rostedt]

On Wed, 2011-04-27 at 17:46 +0100, Al Viro wrote:
> On Tue, Apr 26, 2011 at 03:49:49PM -0300, Thiago Farina wrote:
> > This macro is arguably more readable than its variants:
> > - !strcmp(a, b)
> > - strcmp(a, b) == 0
> 
> Strongly NACKed.  As far as I'm concerned, it's in the same shitbucket as
> bcopy(3), bzero(3) et.al.  Use idiomatic C; extensions of that kind are
> *bad*, since new developers have to learn them.

What developer has to really learn streq()? I mean it is pretty obvious
to what it does, as suppose to what bzero and bcopy do. A quick google
on "streq" brings up lots of matches of people who already do this.

Actually, some have "optimized" it with:

#define streq(a, b) (*(a) == *(b) && strcmp(a, b) == 0)

As it fails out if the first character does not match (likely the case
if strings do not match) before it calls the strcmp function.

-- Steve

Re: [PATCH] linux/string.h: Introduce streq macro.

[Steven Rostedt]

On Tue, 2011-04-26 at 16:45 -0300, Thiago Farina wrote:
> This macro is arguably more readable than its variants:
> - !strcmp(a, b)
> - strcmp(a, b) == 0

Actually, this was proposed way back in 2002 my Rusty and I did not see
anyone arguing against it. I wonder why it never was incorporated back
then?

http://marc.info/?l=linux-kernel&m=103284339813100&w=2

[ added Cc's of some of those that replied to this thread ]

-- Steve


> Signed-off-by: Thiago Farina <tfransosi@gmail.com>
> ---
>  Changes from v1 (Steven and Alexey review):
>  - Convert from macro to static inline.
>  - Remove the example.
>  - Add the suggested comment by Steven.
> 
>  include/linux/string.h |   15 +++++++++++++++
>  1 files changed, 15 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/string.h b/include/linux/string.h
> index a716ee2..d859bb2 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -134,6 +134,21 @@ extern ssize_t memory_read_from_buffer(void *to, size_t count, loff_t *ppos,
>  			const void *from, size_t available);
>  
>  /**
> + * streq - Are two strings equal?
> + * @a: first string
> + * @b: second string
> + *
> + * Use: streq(a, b)
> + * Instead of: strcmp(a, b) == 0 or !strcmp(a, b)
> + *
> + * This makes the code more readable and less error prone.
> + */
> +static inline int streq(const char *a, const char *b)
> +{
> +	return strcmp(a, b) == 0;
> +}
> +
> +/**
>   * strstarts - does @str start with @prefix?
>   * @str: string to examine
>   * @prefix: prefix to look for.

Re: [PATCH] linux/string.h: Introduce streq macro.

[H. Peter Anvin]

On 04/27/2011 10:49 AM, Steven Rostedt wrote:
> On Tue, 2011-04-26 at 16:45 -0300, Thiago Farina wrote:
>> This macro is arguably more readable than its variants:
>> - !strcmp(a, b)
>> - strcmp(a, b) == 0
> 
> Actually, this was proposed way back in 2002 my Rusty and I did not see
> anyone arguing against it. I wonder why it never was incorporated back
> then?
> 
> http://marc.info/?l=linux-kernel&m=103284339813100&w=2
> 
> [ added Cc's of some of those that replied to this thread ]
> 

Because !strcmp() is idiomatic C.

This is the same kind of stupidity as

#define BEGIN {
#define END   }

It doesn't matter if it is more readable *to you*... learn the language,
please.

	-hpa

Re: [PATCH] linux/string.h: Introduce streq macro.

[Pekka Enberg]

On Wed, Apr 27, 2011 at 9:33 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> Because !strcmp() is idiomatic C.
>
> This is the same kind of stupidity as
>
> #define BEGIN {
> #define END   }

No it's not.

It's the same kind of API extension kstrdup(), for example, is.
Whether or not we should it do it is a separate matter and I think the
only reasonable argument for and against is whether it (a) reduces the
number of bugs, (b) improves code readability significantly, or (c)
generates better code.

                        Pekka

Re: [PATCH] linux/string.h: Introduce streq macro.

[Steven Rostedt]

On Wed, 2011-04-27 at 11:33 -0700, H. Peter Anvin wrote:
> On 04/27/2011 10:49 AM, Steven Rostedt wrote:
> > On Tue, 2011-04-26 at 16:45 -0300, Thiago Farina wrote:
> >> This macro is arguably more readable than its variants:
> >> - !strcmp(a, b)
> >> - strcmp(a, b) == 0
> > 
> > Actually, this was proposed way back in 2002 my Rusty and I did not see
> > anyone arguing against it. I wonder why it never was incorporated back
> > then?
> > 
> > http://marc.info/?l=linux-kernel&m=103284339813100&w=2
> > 
> > [ added Cc's of some of those that replied to this thread ]
> > 
> 
> Because !strcmp() is idiomatic C.
> 
> This is the same kind of stupidity as
> 
> #define BEGIN {
> #define END   }

I argue that this is totally different than your example. Your example
demonstrates changing the syntax of C to simulate another language. This
has nothing to do with simulating any other language. The problem with 
!strcmp() is that it goes against the semantics of C, as '!' means not.
And to think '!' is an equal can get a bit confusing.

It is also the reason we already have two semantics in the kernel for
this:

	!strcmp(a, b) and strcmp(a, b) == 0

Personally, I'm fine with just using strcmp(a, b) == 0, as I have
learned to understand it. And when reading code, I've actually been able
to teach myself !strcmp(a, b) is equality (although with a slight hiccup
in my thought process).

But I still get stuck when I see the use of strcmp(a, b) meaning a != b.
This is where my brain stops completely to analyze if this is really
what the author of the code meant.

> 
> It doesn't matter if it is more readable *to you*... learn the language,
> please.

I have learned the language (it's my mother tongue), but I think
strcmp() is an anomaly of it. It was a mistake that libc never included
a streq(). If it had, we would not even be having this discussion.
Another note is that strcmp is not really part of the language itself as
we must write it ourselves.

Heck, we could add:

#ifndef __HAVE_ARCH_STREQ
/**
 * streq - Test if two strings are equal
 * @cs: One string
 * @ct: Another string
 */
#undef streq
int streq(const char *cs, const char *ct)
{
	unsigned char c1, c2;

	while (1) {
		c1 = *cs++;
		c2 = *ct++;
		if (c1 != c2)
			return 0;
		if (!c1)
			break;
	}
	return 1;
}
EXPORT_SYMBOL(streq);
#endif

And this would be just like adding another helper function.

-- Steve

Re: [PATCH] linux/string.h: Introduce streq macro.

[Steven Rostedt]

On Wed, 2011-04-27 at 21:51 +0300, Pekka Enberg wrote:

> It's the same kind of API extension kstrdup(), for example, is.
> Whether or not we should it do it is a separate matter and I think the
> only reasonable argument for and against is whether it (a) reduces the
> number of bugs,

I did a quick search through the git logs, and found no bug fixes due to
the semantics. At least by the time it got to mainline, they are fixed
(which is a good thing).


>  (b) improves code readability significantly,

This is a matter of preference. I think I would prefer it, but obviously
others do not.


>  or (c)
> generates better code.

If we implement streq() separately from strcmp() it gets slightly
better:

00000000000001be <strcmp>:
 1be:   55                      push   %rbp
 1bf:   48 89 e5                mov    %rsp,%rbp
 1c2:   8a 07                   mov    (%rdi),%al
 1c4:   8a 16                   mov    (%rsi),%dl
 1c6:   48 ff c7                inc    %rdi
 1c9:   48 ff c6                inc    %rsi
 1cc:   38 d0                   cmp    %dl,%al
 1ce:   74 07                   je     1d7 <strcmp+0x19>
 1d0:   19 c0                   sbb    %eax,%eax
 1d2:   83 c8 01                or     $0x1,%eax
 1d5:   eb 06                   jmp    1dd <strcmp+0x1f>
 1d7:   84 c0                   test   %al,%al
 1d9:   75 e7                   jne    1c2 <strcmp+0x4>
 1db:   31 c0                   xor    %eax,%eax
 1dd:   c9                      leaveq 
 1de:   c3                      retq   

00000000000001df <streq>:
 1df:   55                      push   %rbp
 1e0:   48 89 e5                mov    %rsp,%rbp
 1e3:   8a 07                   mov    (%rdi),%al
 1e5:   8a 16                   mov    (%rsi),%dl
 1e7:   48 ff c7                inc    %rdi
 1ea:   48 ff c6                inc    %rsi
 1ed:   38 d0                   cmp    %dl,%al
 1ef:   75 0b                   jne    1fc <streq+0x1d>
 1f1:   84 c0                   test   %al,%al
 1f3:   75 ee                   jne    1e3 <streq+0x4>
 1f5:   b8 01 00 00 00          mov    $0x1,%eax
 1fa:   eb 02                   jmp    1fe <streq+0x1f>
 1fc:   31 c0                   xor    %eax,%eax
 1fe:   c9                      leaveq 
 1ff:   c3                      retq   


-- Steve

Re: [PATCH] linux/string.h: Introduce streq macro.

[Steven Rostedt]

On Wed, 2011-04-27 at 15:16 -0400, Steven Rostedt wrote:
> On Wed, 2011-04-27 at 21:51 +0300, Pekka Enberg wrote:

> 00000000000001df <streq>:
>  1df:   55                      push   %rbp
>  1e0:   48 89 e5                mov    %rsp,%rbp
>  1e3:   8a 07                   mov    (%rdi),%al
>  1e5:   8a 16                   mov    (%rsi),%dl
>  1e7:   48 ff c7                inc    %rdi
>  1ea:   48 ff c6                inc    %rsi
>  1ed:   38 d0                   cmp    %dl,%al
>  1ef:   75 0b                   jne    1fc <streq+0x1d>
>  1f1:   84 c0                   test   %al,%al
>  1f3:   75 ee                   jne    1e3 <streq+0x4>
>  1f5:   b8 01 00 00 00          mov    $0x1,%eax
>  1fa:   eb 02                   jmp    1fe <streq+0x1f>
>  1fc:   31 c0                   xor    %eax,%eax
>  1fe:   c9                      leaveq 
>  1ff:   c3                      retq   

I just noticed that 5 byte move instruction. So changing the function to
bool, makes it a little nicer:

bool streq(const char *cs, const char *ct)
{
        unsigned char c1, c2;

        while (1) {
                c1 = *cs++;
                c2 = *ct++;
                if (c1 != c2)
                        return false;
                if (!c1)
                        break;
        }
        return true;
}

00000000000001df <streq>:
 1df:   55                      push   %rbp
 1e0:   48 89 e5                mov    %rsp,%rbp
 1e3:   8a 07                   mov    (%rdi),%al
 1e5:   8a 16                   mov    (%rsi),%dl
 1e7:   48 ff c7                inc    %rdi
 1ea:   48 ff c6                inc    %rsi
 1ed:   38 d0                   cmp    %dl,%al
 1ef:   75 08                   jne    1f9 <streq+0x1a>
 1f1:   84 c0                   test   %al,%al
 1f3:   75 ee                   jne    1e3 <streq+0x4>
 1f5:   b0 01                   mov    $0x1,%al
 1f7:   eb 02                   jmp    1fb <streq+0x1c>
 1f9:   31 c0                   xor    %eax,%eax
 1fb:   c9                      leaveq 
 1fc:   c3                      retq   

-- Steve

Re: [PATCH] linux/string.h: Introduce streq macro.

[Pekka Enberg]

On Wed, Apr 27, 2011 at 10:16 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Wed, 2011-04-27 at 21:51 +0300, Pekka Enberg wrote:
>
>> It's the same kind of API extension kstrdup(), for example, is.
>> Whether or not we should it do it is a separate matter and I think the
>> only reasonable argument for and against is whether it (a) reduces the
>> number of bugs,
>
> I did a quick search through the git logs, and found no bug fixes due to
> the semantics. At least by the time it got to mainline, they are fixed
> (which is a good thing).
>
>
>>  (b) improves code readability significantly,
>
> This is a matter of preference. I think I would prefer it, but obviously
> others do not.
>
>
>>  or (c)
>> generates better code.
>
> If we implement streq() separately from strcmp() it gets slightly
> better:

We'd probably end up with both in the tree, though, which is not an
improvement. With kstrdup(), for example, we were able to move code
out-of-line which improved the whole kernel.

To be honest, I don't think the arguments for streq() are that strong
but I wanted to point out that the arguments against it weren't all
that great either...

                        Pekka

Re: [PATCH] linux/string.h: Introduce streq macro.

[Steven Rostedt]

On Wed, 2011-04-27 at 22:38 +0300, Pekka Enberg wrote:

> To be honest, I don't think the arguments for streq() are that strong
> but I wanted to point out that the arguments against it weren't all
> that great either...

And I totally agree with you here. To be honest myself, I'm not set on
having streq() in the kernel. I'm perfectly happy doing the strcmp()==0
method. If someone were to get it into the kernel I would be happy to
convert to it.

But like you, I don't think the strong NACKs were justified. It was a
lot of hand waving why we should not have it in the kernel. If the
argument against streq() is simply that the arguments are not strong
enough to add it, and there's no real evidence that it fixes bugs, then
sure, I'll buy that. And I've been stating from the beginning that this
is all a preference thing. I'm guessing that we have probably the same
amount for it as against it, so I'm against a full conversion to it. But
this talk of it changing the C language is a bunch of bull. It's no
different than having printk() instead of printf().

-- Steve

Re: [PATCH] linux/string.h: Introduce streq macro.

[H. Peter Anvin]

On 04/27/2011 01:04 PM, Steven Rostedt wrote:
> It's no different than having printk() instead of printf().

Which IMO was a mistake in Linux.  It would be different if the priority
argument had been explicit (although these days it's functionally
required, which sort-of justifies a different name.)

kmalloc(), kstrdup() etc. have different names because they're different
interfaces, which don't duplicate the malloc(), strdup() etc. semantics.

	-hpa

Re: [PATCH] linux/string.h: Introduce streq macro.

[Al Viro]

On Wed, Apr 27, 2011 at 01:07:59PM -0400, Steven Rostedt wrote:
> On Wed, 2011-04-27 at 17:46 +0100, Al Viro wrote:
> > On Tue, Apr 26, 2011 at 03:49:49PM -0300, Thiago Farina wrote:
> > > This macro is arguably more readable than its variants:
> > > - !strcmp(a, b)
> > > - strcmp(a, b) == 0
> > 
> > Strongly NACKed.  As far as I'm concerned, it's in the same shitbucket as
> > bcopy(3), bzero(3) et.al.  Use idiomatic C; extensions of that kind are
> > *bad*, since new developers have to learn them.
> 
> What developer has to really learn streq()? I mean it is pretty obvious
> to what it does, as suppose to what bzero and bcopy do. A quick google
> on "streq" brings up lots of matches of people who already do this.

I would.  Coming from BSD background, b* bunch is normal and familiar for
me.  Your streq()...  Nope.  I can use google, but I'd have to stop and
actually do that (and if you bothered to do the same, you would've found
official POSIX manpages for bcopy(3) and friends, complete with
"
    Issue 5

     Moved from X/OPEN UNIX extension to BASE.

    Issue 6

     This function is marked LEGACY.
"
in "CHANGE HISTORY" section.  These are fairly common BSDisms, and if you
grep through drivers/staging you'll find more than one instance in there;
all are deletion fodder).

That's the whole fucking _point_; adding random extensions to the language
leads to the place where Pascal and LISP are and it's not pretty.  Each
might make sense taken separately (hell, bzero(3) would prevent real, honest
to Cthulhu bugs - it's memset(p, 0, n) and we had memset-with-swapped-arguments
bugs fairly often and yes, in our tree most of memset() callers do pass '\0'
as the second argument).  Pile enough of those together and you've got yourself
a dialect only you understand.  _Bad_ idea, since the next thing that happens
is different dialects in different parts of tree.  And the end of non-incestous
code review and fixes.  I've seen it first-hand (OK, second - I had enough
sense to stay out of that particular clusterfuck) on Algol 68 codebase.  I
*really*, *really* do not want to see anything similar ever again.  Especially
on projects I can't just piss upon and walk away from.  The fact that in C
you *can* extend the language that way doesn't make it a good idea.

While we are at it, strcmp() is, indeed, a part of the language.  See
section 7.21.4.2 in C99.

Re: [PATCH] linux/string.h: Introduce streq macro.

[Thiago Farina]

On Wed, Apr 27, 2011 at 6:46 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> That's the whole fucking _point_; adding random extensions to the language
> leads to the place where Pascal and LISP are and it's not pretty.  Each
> might make sense taken separately (hell, bzero(3) would prevent real, honest
> to Cthulhu bugs - it's memset(p, 0, n) and we had memset-with-swapped-arguments
> bugs fairly often and yes, in our tree most of memset() callers do pass '\0'
> as the second argument).  Pile enough of those together and you've got yourself
> a dialect only you understand.  _Bad_ idea, since the next thing that happens
> is different dialects in different parts of tree.  And the end of non-incestous
> code review and fixes.  I've seen it first-hand (OK, second - I had enough
> sense to stay out of that particular clusterfuck) on Algol 68 codebase.  I
> *really*, *really* do not want to see anything similar ever again.  Especially
> on projects I can't just piss upon and walk away from.  The fact that in C
> you *can* extend the language that way doesn't make it a good idea.
>
> While we are at it, strcmp() is, indeed, a part of the language.

Part of the C standard library you mean, no?

> See
> section 7.21.4.2 in C99.
>

Re: [PATCH] linux/string.h: Introduce streq macro.

[Thiago Farina]

On Wed, Apr 27, 2011 at 2:07 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Wed, 2011-04-27 at 17:46 +0100, Al Viro wrote:
>> On Tue, Apr 26, 2011 at 03:49:49PM -0300, Thiago Farina wrote:
>> > This macro is arguably more readable than its variants:
>> > - !strcmp(a, b)
>> > - strcmp(a, b) == 0
>>
>> Strongly NACKed.  As far as I'm concerned, it's in the same shitbucket as
>> bcopy(3), bzero(3) et.al.  Use idiomatic C; extensions of that kind are
>> *bad*, since new developers have to learn them.
>
> What developer has to really learn streq()?
Yeah, I'm not understanding why they can understand !strcmp(a,b) but
not streq ;)

Also, I know it doesn't matter but we already have one definition of
streq on scripts/dtc/dtc.h

> I mean it is pretty obvious to what it does, as suppose to what bzero and bcopy do. A quick google
> on "streq" brings up lots of matches of people who already do this.
>
> Actually, some have "optimized" it with:
>
> #define streq(a, b) (*(a) == *(b) && strcmp(a, b) == 0)
>
> As it fails out if the first character does not match (likely the case
> if strings do not match) before it calls the strcmp function.
>
> -- Steve
>
>
>

Re: [PATCH] linux/string.h: Introduce streq macro.

[Pavel Vasilyev]

[-- Attachment #1: Type: text/plain, Size: 215 bytes --]

28.04.2011 02:17, Thiago Farina пишет:
> Part of the C standard library you mean, no?


Library is just a bunch of commonly used techniques

-- 

                                                         Pavel.

[-- Attachment #2: 0x03742489.asc --]
[-- Type: application/pgp-keys, Size: 2524 bytes --]

Re: [PATCH] linux/string.h: Introduce streq macro.

[Al Viro]

On Wed, Apr 27, 2011 at 07:17:11PM -0300, Thiago Farina wrote:
> > While we are at it, strcmp() is, indeed, a part of the language.
> 
> Part of the C standard library you mean, no?

Which is a part of the language.  Sure, our implementation happens to be
freestanding.  Most of C programmers you could find would have worked
with hosted implementations and would be familiar with the damn thing.

> >??See
> > section 7.21.4.2 in C99.

Re: [PATCH] linux/string.h: Introduce streq macro.

[Ted Ts'o]

On Wed, Apr 27, 2011 at 11:33:07AM -0700, H. Peter Anvin wrote:
> 
> Because !strcmp() is idiomatic C.
> 
> This is the same kind of stupidity as
> 
> #define BEGIN {
> #define END   }
> 
> It doesn't matter if it is more readable *to you*... learn the language,
> please.

Interesting bit of trivia:  This sort of nonsense[1] is what inspired
the the International Obfuscated C Code Contest[2].  :-)

[1] http://minnie.tuhs.org/cgi-bin/utree.pl?file=V7/usr/src/cmd/sh/mac.h
[2] http://www.ioccc.org/faq.html

						- Ted

Re: [PATCH] linux/string.h: Introduce streq macro.

[Steven Rostedt]

On Wed, 2011-04-27 at 22:46 +0100, Al Viro wrote:

> That's the whole fucking _point_; adding random extensions to the language
> leads to the place where Pascal and LISP are and it's not pretty.  Each
> might make sense taken separately (hell, bzero(3) would prevent real, honest
> to Cthulhu bugs - it's memset(p, 0, n) and we had memset-with-swapped-arguments
> bugs fairly often and yes, in our tree most of memset() callers do pass '\0'
> as the second argument).  Pile enough of those together and you've got yourself
> a dialect only you understand.  _Bad_ idea, since the next thing that happens
> is different dialects in different parts of tree.  And the end of non-incestous
> code review and fixes.  I've seen it first-hand (OK, second - I had enough
> sense to stay out of that particular clusterfuck) on Algol 68 codebase.  I
> *really*, *really* do not want to see anything similar ever again.  Especially
> on projects I can't just piss upon and walk away from.  The fact that in C
> you *can* extend the language that way doesn't make it a good idea.
> 

So in translating the above I have:

"Don't do this because you are opening up a door that will lead to
extension hell"

Am I correct?

If I am, then I will take that as a reason not to add it and leave it at
that.

-- Steve

Re: [PATCH] linux/string.h: Introduce streq macro.

[Rusty Russell]

On Wed, 27 Apr 2011 11:33:07 -0700, "H. Peter Anvin" <hpa@zytor.com> wrote:
> On 04/27/2011 10:49 AM, Steven Rostedt wrote:
> > On Tue, 2011-04-26 at 16:45 -0300, Thiago Farina wrote:
> >> This macro is arguably more readable than its variants:
> >> - !strcmp(a, b)
> >> - strcmp(a, b) == 0
> > 
> > Actually, this was proposed way back in 2002 my Rusty and I did not see
> > anyone arguing against it. I wonder why it never was incorporated back
> > then?
> > 
> > http://marc.info/?l=linux-kernel&m=103284339813100&w=2
> > 
> > [ added Cc's of some of those that replied to this thread ]
> > 
> 
> Because !strcmp() is idiomatic C.

I proposed it because I *did* find a bug caused by my own misuse of it.
Only once in 15 years as an experienced C coder, but a bug is a bug.

But why argue; #define it in your code if you want.  If enough people
do, we'll want to unify it.

Personally, I think it's marginal: only those with enough knowledge to
avoid the trap anyway will know to use it, and YA kernel-specific piece
of knowledge cancels the readability benefit.

But who knows, maybe it'll catch on elsewhere too?  That would be a win.

> It doesn't matter if it is more readable *to you*... learn the language,
> please.

That API is crap: insulting the user makes us look foolish.

And even experienced coders can get hit by bad APIs.  The invalidity of
this program shocked me recently:

  #include <ctype.h>
  int main(int argc, char *argv[]) { return isupper(argv[0][0]) ? 1 : 0; }

Thanks,
Rusty.