Re: Using coccinelle for (quick?) syntax fixing
On 14.08.2010 08:57, David Holland wrote: On Wed, Aug 11, 2010 at 11:49:10PM +0200, Jean-Yves Migeon wrote: - esp_aeesctr_mature %s: unsupported algorithm.\n, - algo-name)); + %s: unsupported encryption algorithm %d\n, + __func__, sav-alg_enc)); I would say don't do __func__ for messages like this; it doesn't really serve much purpose vs. typing in a name, causes the observable behavior to change silently if the code gets reorganized, and makes it much harder to grep the source for an error message you're seeing. Understood Changing it to use aprint_whatnot_foo(), though, might be worthwhile... Hmm, I thought aprint_* functions were essentially for autoconf messages? -- Jean-Yves Migeon jeanyves.mig...@free.fr
Re: Using coccinelle for (quick?) syntax fixing
On Sat, Aug 14, 2010 at 01:36:15PM +0200, Jean-Yves Migeon wrote: I would say don't do __func__ for messages like this; it doesn't really serve much purpose vs. typing in a name, causes the observable behavior to change silently if the code gets reorganized, and makes it much harder to grep the source for an error message you're seeing. Understood not a big deal though. Changing it to use aprint_whatnot_foo(), though, might be worthwhile... Hmm, I thought aprint_* functions were essentially for autoconf messages? Erm... um, yeah, never mind, I was thinking of the way they automatically rope in the device name and unit number. -- David A. Holland dholl...@netbsd.org
Re: Using coccinelle for (quick?) syntax fixing
On Thu, 12 Aug 2010, Bernd Ernesti wrote:
if (dev_priv == NULL) {
DRM_ERROR(called with no initialization\n);
- DRM_SPINUNLOCK(dev_priv-cs.cs_mutex);
...
Hmm, you didn't mention why you are doing that in your initial mail.
Use of pointer after determining it's NULL, in thias case dev_priv
(it _was_ in one of the prior mails).
- Hubert
re: Using coccinelle for (quick?) syntax fixing
On Wed, Aug 11, 2010 at 11:49:10PM +0200, Jean-Yves Migeon wrote:
[..]
I will commit the attached patch on Saturday, unless someone objects.
[..]
Index: external/bsd/drm/dist/shared-core/radeon_cs.c
===
RCS file: /cvsroot/src/sys/external/bsd/drm/dist/shared-core/radeon_cs.c,v
retrieving revision 1.1
diff -u -p -r1.1 radeon_cs.c
--- external/bsd/drm/dist/shared-core/radeon_cs.c 24 May 2010 01:39:06
- 1.1
+++ external/bsd/drm/dist/shared-core/radeon_cs.c 11 Aug 2010 19:54:26
-
@@ -150,7 +150,6 @@ int radeon_cs_ioctl(struct drm_device *d
if (dev_priv == NULL) {
DRM_ERROR(called with no initialization\n);
- DRM_SPINUNLOCK(dev_priv-cs.cs_mutex);
return -EINVAL;
}
if (!cs-num_chunks) {
Hmm, you didn't mention why you are doing that in your initial mail.
indeed, this seems entirely wrong. do not commit this part.
.mrg.
re: Using coccinelle for (quick?) syntax fixing
On Thu, 12 Aug 2010, Bernd Ernesti wrote:
if (dev_priv == NULL) {
DRM_ERROR(called with no initialization\n);
- DRM_SPINUNLOCK(dev_priv-cs.cs_mutex);
...
Hmm, you didn't mention why you are doing that in your initial mail.
Use of pointer after determining it's NULL, in thias case dev_priv
(it _was_ in one of the prior mails).
yes, but the pointer has already been derefernced. the above
is not the correct fix.
.mrg.
Re: Using coccinelle for (quick?) syntax fixing
On 12.08.2010 06:49, Bernd Ernesti wrote:
On Wed, Aug 11, 2010 at 11:49:10PM +0200, Jean-Yves Migeon wrote:
[..]
I will commit the attached patch on Saturday, unless someone objects.
[..]
Index: external/bsd/drm/dist/shared-core/radeon_cs.c
===
RCS file: /cvsroot/src/sys/external/bsd/drm/dist/shared-core/radeon_cs.c,v
retrieving revision 1.1
diff -u -p -r1.1 radeon_cs.c
--- external/bsd/drm/dist/shared-core/radeon_cs.c24 May 2010 01:39:06
- 1.1
+++ external/bsd/drm/dist/shared-core/radeon_cs.c11 Aug 2010 19:54:26
-
@@ -150,7 +150,6 @@ int radeon_cs_ioctl(struct drm_device *d
if (dev_priv == NULL) {
DRM_ERROR(called with no initialization\n);
-DRM_SPINUNLOCK(dev_priv-cs.cs_mutex);
return -EINVAL;
}
if (!cs-num_chunks) {
Hmm, you didn't mention why you are doing that in your initial mail.
Indeed, forgot to remove that one. Thanks.
--
Jean-Yves Migeon
jeanyves.mig...@free.fr
Re: Using coccinelle for (quick?) syntax fixing
On 10.08.2010 19:11, Matthias Drochner wrote:
jeanyves.mig...@free.fr said:
But the NULL deref fixes will go in eventually, and I will probably
ask for a pull up too ;)
But then...
algo = esp_algorithm_lookup(sav-alg_enc);
- if (!algo) {
+ if (algo == NULL) {
ipseclog((LOG_ERR,
- esp_cbc_mature %s: unsupported algorithm.\n,
algo-name));
+ esp_cbc_mature: NULL is not a valid algorithm.\n));
return 1;
The new error message if not helpful. How about printing alg_enc
instead which is in a well-defined namespace.
Fixed
I will commit the attached patch on Saturday, unless someone objects.
--
Jean-Yves Migeon
jeanyves.mig...@free.fr
Index: dev/ieee1394/firewire.c
===
RCS file: /cvsroot/src/sys/dev/ieee1394/firewire.c,v
retrieving revision 1.35
diff -u -p -r1.35 firewire.c
--- dev/ieee1394/firewire.c 23 May 2010 18:56:58 - 1.35
+++ dev/ieee1394/firewire.c 11 Aug 2010 19:54:24 -
@@ -907,7 +907,7 @@ fw_xfer_free(struct fw_xfer* xfer)
{
if (xfer == NULL) {
- aprint_error_dev(xfer-fc-bdev, xfer == NULL\n);
+ aprint_error(%s: xfer == NULL\n, __func__);
return;
}
fw_xfer_unload(xfer);
@@ -920,7 +920,7 @@ fw_xfer_free_buf(struct fw_xfer* xfer)
{
if (xfer == NULL) {
- aprint_error_dev(xfer-fc-bdev, xfer == NULL\n);
+ aprint_error(%s: xfer == NULL\n, __func__);
return;
}
fw_xfer_unload(xfer);
Index: external/bsd/drm/dist/shared-core/radeon_cs.c
===
RCS file: /cvsroot/src/sys/external/bsd/drm/dist/shared-core/radeon_cs.c,v
retrieving revision 1.1
diff -u -p -r1.1 radeon_cs.c
--- external/bsd/drm/dist/shared-core/radeon_cs.c 24 May 2010 01:39:06
- 1.1
+++ external/bsd/drm/dist/shared-core/radeon_cs.c 11 Aug 2010 19:54:26
-
@@ -150,7 +150,6 @@ int radeon_cs_ioctl(struct drm_device *d
if (dev_priv == NULL) {
DRM_ERROR(called with no initialization\n);
- DRM_SPINUNLOCK(dev_priv-cs.cs_mutex);
return -EINVAL;
}
if (!cs-num_chunks) {
Index: netinet6/esp_aesctr.c
===
RCS file: /cvsroot/src/sys/netinet6/esp_aesctr.c,v
retrieving revision 1.12
diff -u -p -r1.12 esp_aesctr.c
--- netinet6/esp_aesctr.c 18 Apr 2009 14:58:05 - 1.12
+++ netinet6/esp_aesctr.c 11 Aug 2010 19:54:26 -
@@ -79,10 +79,10 @@ esp_aesctr_mature(struct secasvar *sav)
const struct esp_algorithm *algo;
algo = esp_algorithm_lookup(sav-alg_enc);
- if (!algo) {
+ if (algo == NULL) {
ipseclog((LOG_ERR,
- esp_aeesctr_mature %s: unsupported algorithm.\n,
- algo-name));
+ %s: unsupported encryption algorithm %d\n,
+ __func__, sav-alg_enc));
return 1;
}
Index: netinet6/esp_core.c
===
RCS file: /cvsroot/src/sys/netinet6/esp_core.c,v
retrieving revision 1.45
diff -u -p -r1.45 esp_core.c
--- netinet6/esp_core.c 18 Apr 2009 14:58:05 - 1.45
+++ netinet6/esp_core.c 11 Aug 2010 19:54:27 -
@@ -404,9 +404,10 @@ esp_cbc_mature(struct secasvar *sav)
}
algo = esp_algorithm_lookup(sav-alg_enc);
- if (!algo) {
+ if (algo == NULL) {
ipseclog((LOG_ERR,
- esp_cbc_mature %s: unsupported algorithm.\n, algo-name));
+ %s: unsupported encryption algorithm %d\n,
+ __func__, sav-alg_enc));
return 1;
}
Re: Using coccinelle for (quick?) syntax fixing
On 10.08.2010 19:11, Matthias Drochner wrote:
jeanyves.mig...@free.fr said:
But the NULL deref fixes will go in eventually, and I will probably
ask for a pull up too ;)
But then...
algo = esp_algorithm_lookup(sav-alg_enc);
- if (!algo) {
+ if (algo == NULL) {
ipseclog((LOG_ERR,
- esp_cbc_mature %s: unsupported algorithm.\n,
algo-name));
+ esp_cbc_mature: NULL is not a valid algorithm.\n));
return 1;
The new error message if not helpful. How about printing alg_enc
instead which is in a well-defined namespace.
Fixed
I will commit the attached patch on Saturday, unless someone objects.
--
Jean-Yves Migeon
jeanyves.mig...@free.fr
Index: dev/ieee1394/firewire.c
===
RCS file: /cvsroot/src/sys/dev/ieee1394/firewire.c,v
retrieving revision 1.35
diff -u -p -r1.35 firewire.c
--- dev/ieee1394/firewire.c 23 May 2010 18:56:58 - 1.35
+++ dev/ieee1394/firewire.c 11 Aug 2010 19:54:24 -
@@ -907,7 +907,7 @@ fw_xfer_free(struct fw_xfer* xfer)
{
if (xfer == NULL) {
- aprint_error_dev(xfer-fc-bdev, xfer == NULL\n);
+ aprint_error(%s: xfer == NULL\n, __func__);
return;
}
fw_xfer_unload(xfer);
@@ -920,7 +920,7 @@ fw_xfer_free_buf(struct fw_xfer* xfer)
{
if (xfer == NULL) {
- aprint_error_dev(xfer-fc-bdev, xfer == NULL\n);
+ aprint_error(%s: xfer == NULL\n, __func__);
return;
}
fw_xfer_unload(xfer);
Index: external/bsd/drm/dist/shared-core/radeon_cs.c
===
RCS file: /cvsroot/src/sys/external/bsd/drm/dist/shared-core/radeon_cs.c,v
retrieving revision 1.1
diff -u -p -r1.1 radeon_cs.c
--- external/bsd/drm/dist/shared-core/radeon_cs.c 24 May 2010 01:39:06
- 1.1
+++ external/bsd/drm/dist/shared-core/radeon_cs.c 11 Aug 2010 19:54:26
-
@@ -150,7 +150,6 @@ int radeon_cs_ioctl(struct drm_device *d
if (dev_priv == NULL) {
DRM_ERROR(called with no initialization\n);
- DRM_SPINUNLOCK(dev_priv-cs.cs_mutex);
return -EINVAL;
}
if (!cs-num_chunks) {
Index: netinet6/esp_aesctr.c
===
RCS file: /cvsroot/src/sys/netinet6/esp_aesctr.c,v
retrieving revision 1.12
diff -u -p -r1.12 esp_aesctr.c
--- netinet6/esp_aesctr.c 18 Apr 2009 14:58:05 - 1.12
+++ netinet6/esp_aesctr.c 11 Aug 2010 19:54:26 -
@@ -79,10 +79,10 @@ esp_aesctr_mature(struct secasvar *sav)
const struct esp_algorithm *algo;
algo = esp_algorithm_lookup(sav-alg_enc);
- if (!algo) {
+ if (algo == NULL) {
ipseclog((LOG_ERR,
- esp_aeesctr_mature %s: unsupported algorithm.\n,
- algo-name));
+ %s: unsupported encryption algorithm %d\n,
+ __func__, sav-alg_enc));
return 1;
}
Index: netinet6/esp_core.c
===
RCS file: /cvsroot/src/sys/netinet6/esp_core.c,v
retrieving revision 1.45
diff -u -p -r1.45 esp_core.c
--- netinet6/esp_core.c 18 Apr 2009 14:58:05 - 1.45
+++ netinet6/esp_core.c 11 Aug 2010 19:54:27 -
@@ -404,9 +404,10 @@ esp_cbc_mature(struct secasvar *sav)
}
algo = esp_algorithm_lookup(sav-alg_enc);
- if (!algo) {
+ if (algo == NULL) {
ipseclog((LOG_ERR,
- esp_cbc_mature %s: unsupported algorithm.\n, algo-name));
+ %s: unsupported encryption algorithm %d\n,
+ __func__, sav-alg_enc));
return 1;
}
Re: Using coccinelle for (quick?) syntax fixing
On Mon, Aug 09, 2010 at 10:49:05PM +0200, Jean-Yves Migeon wrote: Note also that I applied the spatch against sys/; the rest of src could get a scan. But I would prefer to look for other static analyzers first, perhaps there are more suitable (and faster) ones. If you're talking about static analyzers, wouldn't coverity count? I haven't actually used Coccinelle, just read about it, but it seems like it's a tool for modifying the sources, rather than finding the problems in the first place, and trying to make it do the latter sounds difficult. eric
Re: Using coccinelle for (quick?) syntax fixing
jeanyves.mig...@free.fr said:
But the NULL deref fixes will go in eventually, and I will probably
ask for a pull up too ;)
But then...
algo = esp_algorithm_lookup(sav-alg_enc);
- if (!algo) {
+ if (algo == NULL) {
ipseclog((LOG_ERR,
- esp_cbc_mature %s: unsupported algorithm.\n, algo-name));
+ esp_cbc_mature: NULL is not a valid algorithm.\n));
return 1;
The new error message if not helpful. How about printing alg_enc
instead which is in a well-defined namespace.
best regards
Matthias
Forschungszentrum Juelich GmbH
52425 Juelich
Sitz der Gesellschaft: Juelich
Eingetragen im Handelsregister des Amtsgerichts Dueren Nr. HR B 3498
Vorsitzender des Aufsichtsrats: MinDirig Dr. Karl Eugen Huthmacher
Geschaeftsfuehrung: Prof. Dr. Achim Bachem (Vorsitzender),
Dr. Ulrich Krafft (stellv. Vorsitzender), Prof. Dr.-Ing. Harald Bolt,
Prof. Dr. Sebastian M. Schmidt
Re: Using coccinelle for (quick?) syntax fixing
On Tue, 10 Aug 2010 12:11:32 -0500, Eric Haszlakiewicz e...@nimenees.com wrote: On Mon, Aug 09, 2010 at 10:49:05PM +0200, Jean-Yves Migeon wrote: Note also that I applied the spatch against sys/; the rest of src could get a scan. But I would prefer to look for other static analyzers first, perhaps there are more suitable (and faster) ones. If you're talking about static analyzers, wouldn't coverity count? Yes $ cd /usr/pkgsrc/*/coverity/ ksh: cd: /usr/pkgsrc/*/coverity - No such file or directory :( I haven't actually used Coccinelle, just read about it, but it seems like it's a tool for modifying the sources, rather than finding the problems in the first place, and trying to make it do the latter sounds difficult. Not difficult; in these examples, it is just plain overkill: a decent static analyzer is likely to catch them. Coccinelle is rather a tool to simplify code overhaul, something you cannot do with coverity-like things. I will give a more concrete example with my kvm(3) patch for 64 bits paddr_t and i386, but it is not ready yet. -- Jean-Yves Migeon jeanyves.mig...@free.fr
Re: Using coccinelle for (quick?) syntax fixing
On Tue, 10 Aug 2010 19:11:56 +0200, Matthias Drochner
m.droch...@fz-juelich.de wrote:
jeanyves.mig...@free.fr said:
But the NULL deref fixes will go in eventually, and I will probably
ask for a pull up too ;)
But then...
algo = esp_algorithm_lookup(sav-alg_enc);
- if (!algo) {
+ if (algo == NULL) {
ipseclog((LOG_ERR,
- esp_cbc_mature %s: unsupported algorithm.\n,
algo-name));
+ esp_cbc_mature: NULL is not a valid
algorithm.\n));
return 1;
The new error message if not helpful. How about printing alg_enc
instead which is in a well-defined namespace.
Sure.
I invite others to comment on these small patches...
--
Jean-Yves Migeon
jeanyves.mig...@free.fr
Re: Using coccinelle for (quick?) syntax fixing
On Tue, Aug 10, 2010 at 07:40:38PM +0200, Jean-Yves Migeon wrote: On Tue, 10 Aug 2010 12:11:32 -0500, Eric Haszlakiewicz e...@nimenees.com wrote: On Mon, Aug 09, 2010 at 10:49:05PM +0200, Jean-Yves Migeon wrote: Note also that I applied the spatch against sys/; the rest of src could get a scan. But I would prefer to look for other static analyzers first, perhaps there are more suitable (and faster) ones. If you're talking about static analyzers, wouldn't coverity count? Yes $ cd /usr/pkgsrc/*/coverity/ ksh: cd: /usr/pkgsrc/*/coverity - No such file or directory :( It wouldn't do you any good even if it was there: if you're going to run it yourself you need to buy a license. Even if you could run it yourself you wouldn't have the benefit of using the existing, pre-classified list of defects on coverity's site. eric
Re: Using coccinelle for (quick?) syntax fixing
On Sun, 8 Aug 2010 14:45:59 -0400, Matthew Mondor mm_li...@pulsar-zone.net wrote: On Sun, 08 Aug 2010 18:05:11 +0200 Jean-Yves Migeon jeanyves.mig...@free.fr wrote: Opinions? Any interest in it? My intent is to put NetBSD specific scripts on wiki.n.o, and provide links for more generic ones. That seems like a handy tool to save time and avoid a number of typos, if it's used right. Thanks for sharing, I personally didn't know Coccinelle. And example scripts can often be more useful than plain documentation, especially if it's in a WIP state (I liked that they showed in a few lines why it's better than sed :)) Yeah, the documentation is a WIP, and is rather difficult to follow; starting with SmPL grammar is one thing, but some options are clearly not documented, and hard to guess (I am not at all familiar with ocaml). I spent approx. 1 hour to figure out how I could print file name and line with the aprint thing, and never found a solution for pattern matching on expression (detect %s: ). Examples on the site _do_ help there. It is 'error-prone, in the sense that it can raise false positives. But when you get more familiar with it, you can either fix the cocci patch (easy for __arraycount, I missed one of the cases... less obvious for aprint stuff), and proof read the generated patch. I used these examples to get familiar with it; it starts getting useful when you try to find out buggy code, like double free() in the same function, mutex_exit() missing in a branch before returning, etc. -- Jean-Yves Migeon jeanyves.mig...@free.fr
Re: Using coccinelle for (quick?) syntax fixing
On Mon Aug 09 2010 at 11:20:29 +0200, Jean-Yves Migeon wrote: It is 'error-prone, in the sense that it can raise false positives. But when you get more familiar with it, you can either fix the cocci patch (easy for __arraycount, I missed one of the cases... less obvious for aprint stuff), and proof read the generated patch. I really dislike untested wide-angle churn, especially if there is 0 measurable gain. Converting code to __arraycount is a prime example. The only benefit of __arraycount is avoiding typing and therefore typos. Neither of those apply when doing a churn. (there are subjective beauty values, but every C programmer knows the sizeof/sizeof idiom, which is more than what can be said about __arraycount) Examples of measurable benefit are good. Encouraging churn is less good, even if spatch-churn is a million times better than sed-churn. I used these examples to get familiar with it; it starts getting useful when you try to find out buggy code, like double free() in the same function, mutex_exit() missing in a branch before returning, etc. Static analysis is good. However, it might take quite a bit of effort to get the rules general enough so that they trigger in more than one file and specific enough so that you don't get too many false positives. Just to give an example, the ffs allocator routines don't release the lock in an error branch. I remember coccinelle had problems with cpp. Any code which uses macros to skip C syntax will fail silently. procfs comes to mind here. Also, I remember it using so much memory when given our kernel source that I could not finish a rototill and had to use it in combo with find/grep. That said, if $someone can produce a set of rules which showably find bugs in NetBSD code and do not produce a lot of false positives, I'm very interested in seeing nightly runs. ... especially if there are no TAILQ false positives ;)
Re: Using coccinelle for (quick?) syntax fixing
On Mon, 9 Aug 2010 13:27:38 +0300, Antti Kantee po...@cs.hut.fi wrote: I really dislike untested wide-angle churn, especially if there is 0 measurable gain. Converting code to __arraycount is a prime example. The only benefit of __arraycount is avoiding typing and therefore typos. Neither of those apply when doing a churn. (there are subjective beauty values, but every C programmer knows the sizeof/sizeof idiom, which is more than what can be said about __arraycount) arraycount is just one example; you could say the same about roundup/howmany/min/max/... For arraycount, the use is limited; I agree. However, I would prefer to have code using roundup/MIN/MAX rather than rewriting them down. It tends to be harder to read, no matter the developer's expertise can be. Examples of measurable benefit are good. Encouraging churn is less good, even if spatch-churn is a million times better than sed-churn. Well, that's the eternal if it ain't broke, don't fix it debate. I don't know about what you mean by measurable benefit. Does code quality, DRY, or KNF fit in? I hardly see how the thing can go wrong in this /very/ simple case though: dependency on cdefs, exact semantic match. Anyway, that's not the purpose here. I used these examples to get familiar with it; it starts getting useful when you try to find out buggy code, like double free() in the same function, mutex_exit() missing in a branch before returning, etc. Static analysis is good. However, it might take quite a bit of effort to get the rules general enough so that they trigger in more than one file and specific enough so that you don't get too many false positives. Just to give an example, the ffs allocator routines don't release the lock in an error branch. That's not because there are specific cases where the tool has limited use that it does not fit the rest :) I remember coccinelle had problems with cpp. Any code which uses macros to skip C syntax will fail silently. procfs comes to mind here. Also, I remember it using so much memory when given our kernel source that I could not finish a rototill and had to use it in combo with find/grep. Indeed, using spatch -dir /usr/cvs/src is quite violent. And does not scale very well when the cases are too general, and need heavy pattern lookup. That said, if $someone can produce a set of rules which showably find bugs in NetBSD code and do not produce a lot of false positives, I'm very interested in seeing nightly runs. Alright, let's get a bit more practical. Warning: patch may not apply cleanly, and I am working with a month-old src: - logical not with bitwise (attached: notvsand.diff) This one should be checked, I am not familiar with this code. - more serious: NULL check then dereference (attached: nullderef.diff) IMHO, the last ones would be easier to spot with if (foo == NULL)... instead of having if (!foo)... Clarity does help (guess it did not for the other half, anyway :/ ) ... especially if there are no TAILQ false positives ;) Only true negatives with that one :p Purpose was to gather opinions (if it got wildly rejected, I would have stop doing anything with it now). Note that I view coccinelle more like a tool making rototilling easier than full blown static analyzer; there are probably better ones out there (clang). -- Jean-Yves Migeon jeanyves.mig...@free.fr notvsand.diff Description: Binary data nullderef.diff Description: Binary data
Re: Using coccinelle for (quick?) syntax fixing
On 09.08.2010 18:45, Antti Kantee wrote: On Mon Aug 09 2010 at 18:19:28 +0200, Jean-Yves Migeon wrote: That said, if $someone can produce a set of rules which showably find bugs in NetBSD code and do not produce a lot of false positives, I'm very interested in seeing nightly runs. Alright, let's get a bit more practical. Warning: patch may not apply cleanly, and I am working with a month-old src: - logical not with bitwise (attached: notvsand.diff) This one should be checked, I am not familiar with this code. Those definitely look fishy. I'm surprised gcc doesn't give a warning. Does not seem to... - more serious: NULL check then dereference (attached: nullderef.diff) IMHO, the last ones would be easier to spot with if (foo == NULL)... instead of having if (!foo)... Clarity does help (guess it did not for the other half, anyway :/ ) Well, at least the first one is under #ifdef __FreeBSD__, and would panic anyway already in malloc ;) Keep looking down a bit :) IMHO, all the rest are real bugs, and concern kernel directly. It affects parts I am sadly not experienced enough with, so I wouldn't commit without prior approval. Note also that I applied the spatch against sys/; the rest of src could get a scan. But I would prefer to look for other static analyzers first, perhaps there are more suitable (and faster) ones. IIRC, there were some private discussions earlier (year or so ago?) and it was suggested that rototills should accompany a public spatch. But given that in practise applying the spatch is usually a combination of using spatch, sed, etc., there would need to be accompanying documentation too. Anyway, someone needs to work out the details and drive the issue. I think your examples are useful to run every now and then in a static analyzer capacity. Just because there are better ones, doesn't mean others are obsolete, especially if the results are different. This requires maintenance, and having multiple tools each raising false positives can only get semi-automated. Besides, avoiding false positive may get the opposite result, masking true ones. -- Jean-Yves Migeon jeanyves.mig...@free.fr
Re: Using coccinelle for (quick?) syntax fixing
Date:Mon, 09 Aug 2010 18:19:28 +0200 From:Jean-Yves Migeon jeanyves.mig...@free.fr Message-ID: e2568087e60b3c2943d03413cd7e5...@localhost I agree with pooka, no code changes to existing code just to make the code look better. | However, I would prefer to have code using roundup/MIN/MAX rather than | rewriting them down. It tends to be harder to read, no matter the | developer's expertise can be. If you're developing new code, or even making changes to existing code for some other reason, then fine - but changing the code just because you can't think of anything else to do, is not useful, however much you would prefer it if the code had been written in a different style. | Does code quality, DRY, or KNF fit in? No. If the code is there, and works as it is supposed to, then leave it alone. If it needs changing for some reason (adding features, removing old stuff that is no longer wanted, fixing bugs, improving performance, ...) then change it as needed. Otherwise, leave it alone. | I hardly see how the thing can go wrong in this /very/ | simple case though: dependency on cdefs, exact semantic match. One obvious example is that it makes it much harder in the future to pullups of fixes to code in this area to earlier NetBSD versions - the diffs show all of this meaningless churn as well as the real change. The same for anyone looking to see why something that used to work no longer does - you might be sure that your changes cannot possibly do any harm, but all someone else sees is a whole bunch of files changed between the version that worked last week, and the new version that doesn't work this week, that they're going to have to filter through to figure out what change actually broke things, even if your change was not responsible (and it probably wouldn't be) you've caused lots of needless unnecessary work. Please don't. kre ps: this has nothing whatever to do with the tool you're using to assist, whether this is done by hand, or automated, makes no real difference.
Re: Using coccinelle for (quick?) syntax fixing
On Sun, 08 Aug 2010 18:05:11 +0200 Jean-Yves Migeon jeanyves.mig...@free.fr wrote: Opinions? Any interest in it? My intent is to put NetBSD specific scripts on wiki.n.o, and provide links for more generic ones. That seems like a handy tool to save time and avoid a number of typos, if it's used right. Thanks for sharing, I personally didn't know Coccinelle. And example scripts can often be more useful than plain documentation, especially if it's in a WIP state (I liked that they showed in a few lines why it's better than sed :)) -- Matt
