| Message ID | cover.1555487380.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | remove extern from function declarations | expand |
On Wed, Apr 17, 2019 at 12:58:31AM -0700, Denton Liu wrote: > Thanks for the feedback. I couldn't find a tool that could selectively > fix indentation on patches so I went through and manually realigned the > parameter lists wherever the tools mangled the alignment. I guess this > also implies that one pair of (tired) human eyes has manually inspected > the machine-generated diff. > > Hopefully, patch 3/4 won't be as onerous to review as it was to write ;) Thanks. I looked over these manually and didn't find any problems (which isn't to say there aren't any, but now at least two sets of tired eyes looked at them :) ). This kind of cleanup is painful, but at least it should be done with after this, so I'm in favor of moving it forward. Two minor notes, though: > compat/mingw.c | 2 +- > compat/mingw.h | 6 +- > compat/nedmalloc/malloc.c.h | 6 +- > compat/obstack.h | 14 +- > compat/poll/poll.h | 2 +- > compat/regex/regex.h | 66 ++--- > compat/win32/pthread.h | 8 +- We sometimes avoid touching compat/ code for style issues because it's copied from elsewhere. And diverging from upstream is more evil than a pure style issue. So potentially we could drop these hunks (though I think maybe mingw is our own thing?). > contrib/coccinelle/noextern.cocci | 6 + I have mixed feelings on this cocci script. I'm happy to _see_ it, as it's important to show how the transformation was done. But for most of the other scripts, we expect programmers to introduce new cases that need converting, and we'd like to catch those automatically. Here I find it reasonably unlikely for a lot of "extern" to slip in, with the exception of some topics in flight. And these coccinelle scripts are kind of expensive to run. So I wonder if the tradeoff is worth it here (perhaps it is now, as we catch those topics in flight, it might be worth dropping this one in a few months). At any rate, thanks for doing all of this tedious work. :) -Peff
On Mon, Apr 22, 2019 at 05:49:01PM -0400, Jeff King wrote: > On Wed, Apr 17, 2019 at 12:58:31AM -0700, Denton Liu wrote: > > compat/mingw.c | 2 +- > > compat/mingw.h | 6 +- > > compat/nedmalloc/malloc.c.h | 6 +- > > compat/obstack.h | 14 +- > > compat/poll/poll.h | 2 +- > > compat/regex/regex.h | 66 ++--- > > compat/win32/pthread.h | 8 +- > > We sometimes avoid touching compat/ code for style issues because it's > copied from elsewhere. And diverging from upstream is more evil than a > pure style issue. So potentially we could drop these hunks (though I > think maybe mingw is our own thing?). > > > contrib/coccinelle/noextern.cocci | 6 + > > I have mixed feelings on this cocci script. I have actual bad experience with this :) v4 of this patch series excluded 'compat/' from the conversion, but the semantic patch is applied to 'compat/' all the same, resulting in failed CI builds because of the four 'extern's in 'compat/obstack.h', and will continue to do so. (Coccinelle has no issues with those other header files; I guess those are not included in the '.c' source files we analyze with Coccinelle in a stock Linux build environment). > I'm happy to _see_ it, as > it's important to show how the transformation was done. But for most of > the other scripts, we expect programmers to introduce new cases that > need converting, and we'd like to catch those automatically. Here I find > it reasonably unlikely for a lot of "extern" to slip in, with the > exception of some topics in flight. > > And these coccinelle scripts are kind of expensive to run. So I wonder > if the tradeoff is worth it here (perhaps it is now, as we catch those > topics in flight, it might be worth dropping this one in a few months). > > At any rate, thanks for doing all of this tedious work. :) > > -Peff
On Thu, Apr 25, 2019 at 02:07:58PM +0200, SZEDER Gábor wrote: > On Mon, Apr 22, 2019 at 05:49:01PM -0400, Jeff King wrote: > > On Wed, Apr 17, 2019 at 12:58:31AM -0700, Denton Liu wrote: > > > > compat/mingw.c | 2 +- > > > compat/mingw.h | 6 +- > > > compat/nedmalloc/malloc.c.h | 6 +- > > > compat/obstack.h | 14 +- > > > compat/poll/poll.h | 2 +- > > > compat/regex/regex.h | 66 ++--- > > > compat/win32/pthread.h | 8 +- > > > > We sometimes avoid touching compat/ code for style issues because it's > > copied from elsewhere. And diverging from upstream is more evil than a > > pure style issue. So potentially we could drop these hunks (though I > > think maybe mingw is our own thing?). > > > > > contrib/coccinelle/noextern.cocci | 6 + > > > > I have mixed feelings on this cocci script. > > I have actual bad experience with this :) > > v4 of this patch series excluded 'compat/' from the conversion, but > the semantic patch is applied to 'compat/' all the same, resulting in > failed CI builds because of the four 'extern's in 'compat/obstack.h', > and will continue to do so. > > (Coccinelle has no issues with those other header files; I guess those > are not included in the '.c' source files we analyze with Coccinelle > in a stock Linux build environment). Since this is the case, we should drop 4/4 because it is not only unhelpful, because it doesn't scan header files, but actively harmful. The cocci script used is in the log for 1/4 anyway. Thanks for checking on this, Denton > > > > I'm happy to _see_ it, as > > it's important to show how the transformation was done. But for most of > > the other scripts, we expect programmers to introduce new cases that > > need converting, and we'd like to catch those automatically. Here I find > > it reasonably unlikely for a lot of "extern" to slip in, with the > > exception of some topics in flight. > > > > And these coccinelle scripts are kind of expensive to run. So I wonder > > if the tradeoff is worth it here (perhaps it is now, as we catch those > > topics in flight, it might be worth dropping this one in a few months). > > > > At any rate, thanks for doing all of this tedious work. :) > > > > -Peff
Hi, On Thu, 25 Apr 2019, SZEDER Gábor wrote: > On Mon, Apr 22, 2019 at 05:49:01PM -0400, Jeff King wrote: > > On Wed, Apr 17, 2019 at 12:58:31AM -0700, Denton Liu wrote: > > > > compat/mingw.c | 2 +- > > > compat/mingw.h | 6 +- > > > compat/nedmalloc/malloc.c.h | 6 +- > > > compat/obstack.h | 14 +- > > > compat/poll/poll.h | 2 +- > > > compat/regex/regex.h | 66 ++--- > > > compat/win32/pthread.h | 8 +- > > > > We sometimes avoid touching compat/ code for style issues because it's > > copied from elsewhere. And diverging from upstream is more evil than a > > pure style issue. So potentially we could drop these hunks (though I > > think maybe mingw is our own thing?). > > > > > contrib/coccinelle/noextern.cocci | 6 + > > > > I have mixed feelings on this cocci script. > > I have actual bad experience with this :) > > v4 of this patch series excluded 'compat/' from the conversion, but > the semantic patch is applied to 'compat/' all the same, resulting in > failed CI builds because of the four 'extern's in 'compat/obstack.h', > and will continue to do so. Is it not possible to exclude certain directories for certain semantic patches? I guess we could also simply declare that *all* Coccinelle patches should leave `compat/` alone, on the basis that those files are likely coming from some sort of upstream. But then, `compat/mingw.c` and `compat/win32/` seem not to fall into that category... Ciao, Dscho
Hi Johannes, On Tue, Apr 30, 2019 at 07:21:40PM -0400, Johannes Schindelin wrote: > Hi, > > On Thu, 25 Apr 2019, SZEDER Gábor wrote: > > > On Mon, Apr 22, 2019 at 05:49:01PM -0400, Jeff King wrote: > > > On Wed, Apr 17, 2019 at 12:58:31AM -0700, Denton Liu wrote: > > > > > > compat/mingw.c | 2 +- > > > > compat/mingw.h | 6 +- > > > > compat/nedmalloc/malloc.c.h | 6 +- > > > > compat/obstack.h | 14 +- > > > > compat/poll/poll.h | 2 +- > > > > compat/regex/regex.h | 66 ++--- > > > > compat/win32/pthread.h | 8 +- > > > > > > We sometimes avoid touching compat/ code for style issues because it's > > > copied from elsewhere. And diverging from upstream is more evil than a > > > pure style issue. So potentially we could drop these hunks (though I > > > think maybe mingw is our own thing?). > > > > > > > contrib/coccinelle/noextern.cocci | 6 + > > > > > > I have mixed feelings on this cocci script. > > > > I have actual bad experience with this :) > > > > v4 of this patch series excluded 'compat/' from the conversion, but > > the semantic patch is applied to 'compat/' all the same, resulting in > > failed CI builds because of the four 'extern's in 'compat/obstack.h', > > and will continue to do so. > > Is it not possible to exclude certain directories for certain semantic > patches? > > I guess we could also simply declare that *all* Coccinelle patches should > leave `compat/` alone, on the basis that those files are likely coming > from some sort of upstream. But then, `compat/mingw.c` and `compat/win32/` > seem not to fall into that category... > > Ciao, > Dscho Deciding whether this is a good idea is above my paygrade ;) However, if this is a good idea, we could use this patch to make it happen: -- >8 -- Subject: [PATCH] Makefile: filter out compat/ from coccicheck Since most files in compat/ are pulled from external sources, ensure that they do not get modified when we run coccicheck because we do not want them to differ from upstream as much as possible. Make exceptions for mingw.c and win32/*.c as these are files that we have created and not pulled from upstream. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- Makefile | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 9f1b6e8926..b083c038c3 100644 --- a/Makefile +++ b/Makefile @@ -2782,11 +2782,14 @@ check: command-list.h fi C_SOURCES = $(patsubst %.o,%.c,$(C_OBJ)) +COCCI_COMPAT_SOURCES = $(addprefix compat/,mingw.c win32/%) ifdef DC_SHA1_SUBMODULE -COCCI_SOURCES = $(filter-out sha1collisiondetection/%,$(C_SOURCES)) +COCCI_SOURCES := $(filter-out sha1collisiondetection/%,$(C_SOURCES)) else -COCCI_SOURCES = $(filter-out sha1dc/%,$(C_SOURCES)) +COCCI_SOURCES := $(filter-out sha1dc/%,$(C_SOURCES)) endif +COCCI_SOURCES := $(filter-out compat/%,$(COCCI_SOURCES)) +COCCI_SOURCES += $(filter $(COCCI_COMPAT_SOURCES),$(C_SOURCES)) %.cocci.patch: %.cocci $(COCCI_SOURCES) @echo ' ' SPATCH $<; \
On Wed, May 01, 2019 at 06:01:08AM -0400, Denton Liu wrote: > > I guess we could also simply declare that *all* Coccinelle patches should > > leave `compat/` alone, on the basis that those files are likely coming > > from some sort of upstream. But then, `compat/mingw.c` and `compat/win32/` > > seem not to fall into that category... > > Deciding whether this is a good idea is above my paygrade ;) FWIW, this seems like a good idea to me. It's _possible_ there may be some cocci changes that we want to apply even to compat/ (e.g., if we're mechanically fixing up a construct with security implications). But whoever is doing that conversion should probably apply that manually (and we don't typically change imported stuff in the first place, so there's little need to subsequently run the cocci patches). > -- >8 -- > Subject: [PATCH] Makefile: filter out compat/ from coccicheck > [...] The patch itself looks OK to me. -Peff
On Wed, May 01, 2019 at 06:01:08AM -0400, Denton Liu wrote: > > Is it not possible to exclude certain directories for certain semantic > > patches? > > > > I guess we could also simply declare that *all* Coccinelle patches should > > leave `compat/` alone, on the basis that those files are likely coming > > from some sort of upstream. But then, `compat/mingw.c` and `compat/win32/` > > seem not to fall into that category... > > > > Ciao, > > Dscho > > Deciding whether this is a good idea is above my paygrade ;) > > However, if this is a good idea, we could use this patch to make it > happen: > > -- >8 -- > Subject: [PATCH] Makefile: filter out compat/ from coccicheck > > Since most files in compat/ are pulled from external sources, ensure > that they do not get modified when we run coccicheck because we do not > want them to differ from upstream as much as possible. > > Make exceptions for mingw.c and win32/*.c as these are files that we > have created and not pulled from upstream. I'm not sure that we really need these exceptions. C_SOURCES comes from C_OBJ, i.e. it is basically all '*.c' source files that we compile, taking the platform and Makefile knobs into account. On Linux we don't compile 'compat/mingw.c' and 'compat/win32/*.c', so when running 'make coccicheck' on Linux it won't look into these source files anyway, so we don't need these exceptions. On Windows, however... well, is it even possible to build and run Coccinelle on Windows in the first place, with all its OCaml dependencies?! If not, then these exceptions won't do any good. Anyway, if we do want these exceptions, then what about 'compat/win32mmap.c' and 'compat/winansi.c'? They look like "ours" as well. FWIW, out of curiosity I've run 'make coccicheck' on Linux with 'compat/mingw.c' and its friends explicitly added to C_SOURCES, and it seems to work... it even found two places in 'mingw.c' where COPY_ARRAY could replace memcpy() :) > Signed-off-by: Denton Liu <liu.denton@gmail.com> > --- > Makefile | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/Makefile b/Makefile > index 9f1b6e8926..b083c038c3 100644 > --- a/Makefile > +++ b/Makefile > @@ -2782,11 +2782,14 @@ check: command-list.h > fi > > C_SOURCES = $(patsubst %.o,%.c,$(C_OBJ)) > +COCCI_COMPAT_SOURCES = $(addprefix compat/,mingw.c win32/%) > ifdef DC_SHA1_SUBMODULE > -COCCI_SOURCES = $(filter-out sha1collisiondetection/%,$(C_SOURCES)) > +COCCI_SOURCES := $(filter-out sha1collisiondetection/%,$(C_SOURCES)) > else > -COCCI_SOURCES = $(filter-out sha1dc/%,$(C_SOURCES)) > +COCCI_SOURCES := $(filter-out sha1dc/%,$(C_SOURCES)) > endif > +COCCI_SOURCES := $(filter-out compat/%,$(COCCI_SOURCES)) > +COCCI_SOURCES += $(filter $(COCCI_COMPAT_SOURCES),$(C_SOURCES)) > > %.cocci.patch: %.cocci $(COCCI_SOURCES) > @echo ' ' SPATCH $<; \ > -- > 2.21.0.1033.g0e8cc1100c >
Hi, On Thu, 2 May 2019, SZEDER Gábor wrote: > On Wed, May 01, 2019 at 06:01:08AM -0400, Denton Liu wrote: > > > Is it not possible to exclude certain directories for certain semantic > > > patches? > > > > > > I guess we could also simply declare that *all* Coccinelle patches should > > > leave `compat/` alone, on the basis that those files are likely coming > > > from some sort of upstream. But then, `compat/mingw.c` and `compat/win32/` > > > seem not to fall into that category... > > > > > > Ciao, > > > Dscho > > > > Deciding whether this is a good idea is above my paygrade ;) :-) As a software developer, you surely have an opinion, though :-D Thank you for the patch, it is a good conversation starter in the least, and I hope that some variation of it will even make it to `master`. > [...] what about 'compat/win32mmap.c' and 'compat/winansi.c'? They look > like "ours" as well. Indeed, this is probably a good indicator that we'd want this to be an opt-out, rather than an opt-in list. For example, we know that we want to exclude compat/regex/ and compat/poll/ from the `coccicheck` target. > FWIW, out of curiosity I've run 'make coccicheck' on Linux with > 'compat/mingw.c' and its friends explicitly added to C_SOURCES, and it > seems to work... it even found two places in 'mingw.c' where > COPY_ARRAY could replace memcpy() :) TBH I had not even known that those files were excluded from coccicheck by default. I had assumed that all of Git's sources (and not just the Linux-specific ones) were included in the target. Since you *could* include it, I now assume that Coccinelle does not need to follow the `#include`s (otherwise, it would have complained about not finding the `windows.h` header in your setup). If this new assumption is true, I wonder why we cannot make my former assumption true as well: why not include *all* of Git's `.c` files in `coccicheck`? Ciao, Dscho
On Thu, May 02, 2019 at 02:04:22AM +0200, SZEDER Gábor wrote: > On Wed, May 01, 2019 at 06:01:08AM -0400, Denton Liu wrote: [snip] > > > > -- >8 -- > > Subject: [PATCH] Makefile: filter out compat/ from coccicheck > > > > Since most files in compat/ are pulled from external sources, ensure > > that they do not get modified when we run coccicheck because we do not > > want them to differ from upstream as much as possible. > > > > Make exceptions for mingw.c and win32/*.c as these are files that we > > have created and not pulled from upstream. > > I'm not sure that we really need these exceptions. > > C_SOURCES comes from C_OBJ, i.e. it is basically all '*.c' source > files that we compile, taking the platform and Makefile knobs into > account. On Linux we don't compile 'compat/mingw.c' and > 'compat/win32/*.c', so when running 'make coccicheck' on Linux it > won't look into these source files anyway, so we don't need these > exceptions. On Windows, however... well, is it even possible to > build and run Coccinelle on Windows in the first place, with all its > OCaml dependencies?! If not, then these exceptions won't do any good. I assumed that Coccinelle runs on Windows but now that you mention it, Cocci's origin was as a Linux refactoring tool so I'm not really sure if it actually does run. Unfortunately, I don't have a Windows box available to test it out on so let's assume that it doesn't work on Windows, unless someone says otherwise. > > Anyway, if we do want these exceptions, then what about > 'compat/win32mmap.c' and 'compat/winansi.c'? They look like "ours" as > well. Good point, I wasn't really sure which ones were ours so I just went off of what Johannes said. If we decide to keep the exceptions, I'll dig through the log messages to find all of "our" files. > > > FWIW, out of curiosity I've run 'make coccicheck' on Linux with > 'compat/mingw.c' and its friends explicitly added to C_SOURCES, and it > seems to work... it even found two places in 'mingw.c' where > COPY_ARRAY could replace memcpy() :) > Since you mentioned this, shouldn't we run Coccinelle on all of our source files, not just the ones that are compiled? Since the Windows-files aren't checked, they are in a blindspot for us. I guess a point against that would be if one were patching a file that they couldn't even test-compile, then it could be possible that faulty patches are sent, but I guess we have enough people on the mailing list that could verify the patches so I don't think that's a problem. Perhaps we could implement a 'coccicheckall' target which excludes contrib/ but excepts mingw.c and friends?
On Fri, May 03, 2019 at 11:32:32AM +0200, Johannes Schindelin wrote: > Hi, > > On Thu, 2 May 2019, SZEDER Gábor wrote: > > > On Wed, May 01, 2019 at 06:01:08AM -0400, Denton Liu wrote: > > > > Is it not possible to exclude certain directories for certain semantic > > > > patches? > > > > > > > > I guess we could also simply declare that *all* Coccinelle patches should > > > > leave `compat/` alone, on the basis that those files are likely coming > > > > from some sort of upstream. But then, `compat/mingw.c` and `compat/win32/` > > > > seem not to fall into that category... > > > > > > > > Ciao, > > > > Dscho > > > > > > Deciding whether this is a good idea is above my paygrade ;) > > :-) > > As a software developer, you surely have an opinion, though :-D I don't even have an opinion yet. :) > > FWIW, out of curiosity I've run 'make coccicheck' on Linux with > > 'compat/mingw.c' and its friends explicitly added to C_SOURCES, and it > > seems to work... it even found two places in 'mingw.c' where > > COPY_ARRAY could replace memcpy() :) > > TBH I had not even known that those files were excluded from coccicheck by > default. Well, there was this line in the patch context: C_SOURCES = $(patsubst %.o,%.c,$(C_OBJ)) > I had assumed that all of Git's sources (and not just the > Linux-specific ones) were included in the target. > > Since you *could* include it, I now assume that Coccinelle does not need > to follow the `#include`s (otherwise, it would have complained about not > finding the `windows.h` header in your setup). We invoke Coccinelle/spatch with the '--all-includes' option, see the SPATCH_FLAGS make variable. And it does indeed include header files: I've seen it find something to transform in 'cache.h', and then the resulting 'contrib/coccinelle/<whatever>.cocci.patch' included the exact same transformation as many times as the number of files including 'cache.h'... which is a lot :) I don't really know what can cause 'spatch' to error out (besides unknown command line option or non-existing file specified on the command line), and this is all that 'man spatch' has to say: --all-includes causes all available include files to be used Since it explicitly mentions _available_ include files, I would venture to guess that non-available include files are not used, and since it doesn't explicitly mention that such a file causes an error, it doesn't. > If this new assumption is true, I wonder why we cannot make my former > assumption true as well: why not include *all* of Git's `.c` files in > `coccicheck`? > > Ciao, > Dscho
On Fri, May 03, 2019 at 04:42:11PM +0200, SZEDER Gábor wrote: > > Since you *could* include it, I now assume that Coccinelle does not need > > to follow the `#include`s (otherwise, it would have complained about not > > finding the `windows.h` header in your setup). > I don't really know what can cause 'spatch' to error out (besides > unknown command line option or non-existing file specified on the > command line), and this is all that 'man spatch' has to say: > > --all-includes > causes all available include files to be used > > Since it explicitly mentions _available_ include files, I would > venture to guess that non-available include files are not used, and > since it doesn't explicitly mention that such a file causes an error, > it doesn't. Actually, we record Coccinelle's output to stderr in a logfile, and with the Windows-specific source files included it contains thing like: HANDLING: compat/mingw.c (ONCE) TYPE: header conio.h not found (ONCE) TYPE: header wchar.h not found diff = init_defs_builtins: /usr/lib/coccinelle/standard.h HANDLING: compat/winansi.c (ONCE) TYPE: header wingdi.h not found (ONCE) TYPE: header winreg.h not found (ONCE) TYPE: header winternl.h not found (ONCE) TYPE: header ntstatus.h not found So it seems that these missing headers are just ignored, only their absence is noted on stderr. (And just for the record, that log also contained these: HANDLING: http-push.c (ONCE) TYPE: header xmlparse.h not found (ONCE) TYPE: header expat.h not found HANDLING: xdiff/xutils.c (ONCE) TYPE: header limits.h not found (ONCE) TYPE: header assert.h not found which means that even on an avarage Linux box there might be missing include files. This also raises the question whether Coccinelle looks for things to transform in system headers as well (and what if it finds something there)).
On Fri, May 03, 2019 at 04:42:11PM +0200, SZEDER Gábor wrote: > > Since you *could* include it, I now assume that Coccinelle does not need > > to follow the `#include`s (otherwise, it would have complained about not > > finding the `windows.h` header in your setup). > > We invoke Coccinelle/spatch with the '--all-includes' option, see the > SPATCH_FLAGS make variable. And it does indeed include header files: > I've seen it find something to transform in 'cache.h', and then the > resulting 'contrib/coccinelle/<whatever>.cocci.patch' included the > exact same transformation as many times as the number of files > including 'cache.h'... which is a lot :) I think spatch is smart enough not to hit the same header multiple times. But the problem is that we invoke it once per file, so it actually processes cache.h many times. That's slow, but also produces bogus patches. Jacob Keller's patches to collapse this to a single invocation do fix it (and I've used them selectively in the past rather than cleaning up the resulting patch manually ;) ). Sort of tangential to your point, I guess, but it might be a topic to revisit. -Peff
On Fri, May 03, 2019 at 01:45:03PM -0400, Jeff King wrote: > On Fri, May 03, 2019 at 04:42:11PM +0200, SZEDER Gábor wrote: > > > > Since you *could* include it, I now assume that Coccinelle does not need > > > to follow the `#include`s (otherwise, it would have complained about not > > > finding the `windows.h` header in your setup). > > > > We invoke Coccinelle/spatch with the '--all-includes' option, see the > > SPATCH_FLAGS make variable. And it does indeed include header files: > > I've seen it find something to transform in 'cache.h', and then the > > resulting 'contrib/coccinelle/<whatever>.cocci.patch' included the > > exact same transformation as many times as the number of files > > including 'cache.h'... which is a lot :) > > I think spatch is smart enough not to hit the same header multiple > times. But the problem is that we invoke it once per file, so it > actually processes cache.h many times. That's slow, but also produces > bogus patches. > > Jacob Keller's patches to collapse this to a single invocation do fix it > (and I've used them selectively in the past rather than cleaning up the > resulting patch manually ;) ). > > Sort of tangential to your point, I guess, but it might be a topic to > revisit. I simply applied the changes manually to 'cache.h', and rerun 'make contrib/coccinelle/<...>.cocci.patch'. This approach is reliable and has less undesirable side-effects :) and it doesn't happen that often to be a considerable burden.
Jeff King <peff@peff.net> writes: > I think spatch is smart enough not to hit the same header multiple > times. But the problem is that we invoke it once per file, so it > actually processes cache.h many times. That's slow, but also produces > bogus patches. Yes, I've seen this and was a bit irritated myself, but not enough to do something about it myself, yet. > > Jacob Keller's patches to collapse this to a single invocation do fix it > (and I've used them selectively in the past rather than cleaning up the > resulting patch manually ;) ). Ah, that is nice to know. Do we want to resurrect it?
On Fri, May 3, 2019 at 11:09 AM Jeff King <peff@peff.net> wrote: > > On Fri, May 03, 2019 at 04:42:11PM +0200, SZEDER Gábor wrote: > > > > Since you *could* include it, I now assume that Coccinelle does not need > > > to follow the `#include`s (otherwise, it would have complained about not > > > finding the `windows.h` header in your setup). > > > > We invoke Coccinelle/spatch with the '--all-includes' option, see the > > SPATCH_FLAGS make variable. And it does indeed include header files: > > I've seen it find something to transform in 'cache.h', and then the > > resulting 'contrib/coccinelle/<whatever>.cocci.patch' included the > > exact same transformation as many times as the number of files > > including 'cache.h'... which is a lot :) > > I think spatch is smart enough not to hit the same header multiple > times. But the problem is that we invoke it once per file, so it > actually processes cache.h many times. That's slow, but also produces > bogus patches. > > Jacob Keller's patches to collapse this to a single invocation do fix it > (and I've used them selectively in the past rather than cleaning up the > resulting patch manually ;) ). > > Sort of tangential to your point, I guess, but it might be a topic to > revisit. > > -Peff Yea, my patches are much faster. However, they trade off a huge increase in memory consumption, and from what I remember, the failure if you don't have enough RAM was not a good experience. Thanks, Jake
On Sat, May 4, 2019 at 10:28 PM Junio C Hamano <gitster@pobox.com> wrote: > > Jeff King <peff@peff.net> writes: > > > I think spatch is smart enough not to hit the same header multiple > > times. But the problem is that we invoke it once per file, so it > > actually processes cache.h many times. That's slow, but also produces > > bogus patches. > > Yes, I've seen this and was a bit irritated myself, but not enough > to do something about it myself, yet. > > > > Jacob Keller's patches to collapse this to a single invocation do fix it > > (and I've used them selectively in the past rather than cleaning up the > > resulting patch manually ;) ). > > Ah, that is nice to know. Do we want to resurrect it? I remember deciding that the memory cost trade off was too high back then. Thanks, Jake