This macro is arguably more readable than its variants:
- !strcmp(a, b)
- strcmp(a, b) == 0
Signed-off-by: Thiago Farina <[email protected]>
---
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
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 <[email protected]>
Acked-by: Steven Rostedt <[email protected]>
> ---
> 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.
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.
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.
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.
On Tue, Apr 26, 2011 at 4:20 PM, Alexey Dobriyan <[email protected]> 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.
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
On Tue, 2011-04-26 at 16:21 -0300, Thiago Farina wrote:
> On Tue, Apr 26, 2011 at 4:20 PM, Alexey Dobriyan <[email protected]> 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
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 <[email protected]> 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?
This macro is arguably more readable than its variants:
- !strcmp(a, b)
- strcmp(a, b) == 0
Signed-off-by: Thiago Farina <[email protected]>
---
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
On Tue, Apr 26, 2011 at 4:45 PM, Joe Perches <[email protected]> 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 <[email protected]> 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.
+Copying Andrew and Joe.
On Tue, Apr 26, 2011 at 4:45 PM, Thiago Farina <[email protected]> wrote:
> This macro is arguably more readable than its variants:
> - !strcmp(a, b)
> - strcmp(a, b) == 0
>
> Signed-off-by: Thiago Farina <[email protected]>
> ---
> 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
>
>
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
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
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?
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
On Tue, Apr 26, 2011 at 5:27 PM, Davidlohr Bueso <[email protected]> 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
>
>
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 <[email protected]>
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
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 <[email protected]>
>
> 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
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.
On Tue, Apr 26, 2011 at 9:58 PM, Steven Rostedt <[email protected]> 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 [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
>
On Wed, Apr 27, 2011 at 10:29, Miguel Ojeda
<[email protected]> wrote:
> On Tue, Apr 26, 2011 at 9:58 PM, Steven Rostedt <[email protected]> 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 -- [email protected]
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
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 <[email protected]>
(i dont mind converting any of the Blackfin code to use this)
-mike
On Wed, Apr 27, 2011 at 10:42 AM, Geert Uytterhoeven
<[email protected]> wrote:
> On Wed, Apr 27, 2011 at 10:29, Miguel Ojeda
> <[email protected]> wrote:
>> On Tue, Apr 26, 2011 at 9:58 PM, Steven Rostedt <[email protected]> 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 -- [email protected]
>
> 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
>
On Wed, 27 Apr 2011, Christoph Hellwig wrote:
> Date: Wed, 27 Apr 2011 02:47:19 -0400
> From: Christoph Hellwig <[email protected]>
> To: Ted Ts'o <[email protected]>, Thiago Farina <[email protected]>,
> [email protected], Steven Rostedt <[email protected]>
> 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
[email protected]
<>< As a computer I find your faith in technology amusing.
27.04.2011 12:49, Miguel Ojeda пишет:
> On Wed, Apr 27, 2011 at 10:42 AM, Geert Uytterhoeven
> <[email protected]> wrote:
>> On Wed, Apr 27, 2011 at 10:29, Miguel Ojeda
>> <[email protected]> wrote:
>>> On Tue, Apr 26, 2011 at 9:58 PM, Steven Rostedt <[email protected]> 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.
On Wed, Apr 27, 2011 at 04:47:40AM -0400, [email protected] 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
On Wed, Apr 27, 2011 at 5:52 PM, Ted Ts'o <[email protected]> wrote:
> On Wed, Apr 27, 2011 at 04:47:40AM -0400, [email protected] 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.
On Wed, 2011-04-27 at 19:04 +0300, Alexey Dobriyan wrote:
> On Wed, Apr 27, 2011 at 5:52 PM, Ted Ts'o <[email protected]> wrote:
> > On Wed, Apr 27, 2011 at 04:47:40AM -0400, [email protected] 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
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.
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
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 <[email protected]>
> ---
> 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.
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
On Wed, Apr 27, 2011 at 9:33 PM, H. Peter Anvin <[email protected]> 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
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
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
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
On Wed, Apr 27, 2011 at 10:16 PM, Steven Rostedt <[email protected]> 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
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
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
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.
On Wed, Apr 27, 2011 at 6:46 PM, Al Viro <[email protected]> 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.
>
On Wed, Apr 27, 2011 at 2:07 PM, Steven Rostedt <[email protected]> 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
>
>
>
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.
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.
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
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
On Wed, 27 Apr 2011 11:33:07 -0700, "H. Peter Anvin" <[email protected]> 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.