threads / patch / 44282

patchconvert: mark a file-local symbol static

Subject: [PATCH] convert: mark a file-local symbol static

## tl;dr

14 messages between Oct 11, 2016 and Oct 17, 2016. Diffs are folded; open one to read it.

replies: 13people: 7as markdown or json

Ramsay Jones· Oct 11, 2016, 23:46 UTC · lore
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
Hi Lars,

If you need to re-roll your 'ls/filter-process' branch, could you please squash this into commit 85290197 ("convert: add filter.<driver>.process option", 08-10-2016).

Thanks.

ATB, Ramsay Jones

 convert.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to convert.c +1 −1
diff --git a/convert.c b/convert.c
index fe68316..cf30380 100644
--- a/convert.c
+++ b/convert.c
@@ -568,7 +568,7 @@ static void kill_multi_file_filter(struct hashmap *hashmap, struct cmd2process *
 	free(entry);
 }
 
-void stop_multi_file_filter(struct child_process *process)
+static void stop_multi_file_filter(struct child_process *process)
 {
 	sigchain_push(SIGPIPE, SIG_IGN);
 	/* Closing the pipe signals the filter to initiate a shutdown. */

-- 
2.10.0
Lars Schneider· Oct 15, 2016, 15:05 UTC · re: Ramsay Jones · lore

Re: [PATCH] convert: mark a file-local symbol static

Show 9 quoted lines
> On 11 Oct 2016, at 16:46, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
> 
> If you need to re-roll your 'ls/filter-process' branch, could you
> please squash this into commit 85290197
> ("convert: add filter.<driver>.process option", 08-10-2016).
> 
> 
> -void stop_multi_file_filter(struct child_process *process)
> +static void stop_multi_file_filter(struct child_process *process)

Done! Do you have some kind of script to detect these things automatically or do you read the code that carefully?

Thanks, Lars

Ramsay Jones· Oct 15, 2016, 21:01 UTC · re: Lars Schneider · lore

Re: [PATCH] convert: mark a file-local symbol static

On 15/10/16 16:05, Lars Schneider wrote:
>> On 11 Oct 2016, at 16:46, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
[snip]
Show 5 quoted lines
>> -void stop_multi_file_filter(struct child_process *process)
>> +static void stop_multi_file_filter(struct child_process *process)
> 
> Done! Do you have some kind of script to detect these things
> automatically or do you read the code that carefully?
Heh, I'm _far_ too lazy to read the code that carefully. :-D

A combination of 'make sparse' and a perl script (originally posted to the list by Junio) find all of these for me.

ATB, Ramsay Jones

Lars Schneider· Oct 16, 2016, 00:15 UTC · re: Ramsay Jones · lore

Re: [PATCH] convert: mark a file-local symbol static

Show 17 quoted lines
> On 15 Oct 2016, at 14:01, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
> 
> 
> 
> On 15/10/16 16:05, Lars Schneider wrote:
>>> On 11 Oct 2016, at 16:46, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
> [snip]
>>> -void stop_multi_file_filter(struct child_process *process)
>>> +static void stop_multi_file_filter(struct child_process *process)
>> 
>> Done! Do you have some kind of script to detect these things
>> automatically or do you read the code that carefully?
> 
> Heh, I'm _far_ too lazy to read the code that carefully. :-D
> 
> A combination of 'make sparse' and a perl script (originally
> posted to the list by Junio) find all of these for me.

Interesting. Do you have a link to this script? Does it generate false positives? Maybe I can add `make sparse` to the TravisCI build?

Cheers, Lars

Ramsay Jones· Oct 17, 2016, 01:37 UTC · re: Lars Schneider · lore

Re: [PATCH] convert: mark a file-local symbol static

On 16/10/16 01:15, Lars Schneider wrote:
Show 16 quoted lines
>> On 15 Oct 2016, at 14:01, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
>> On 15/10/16 16:05, Lars Schneider wrote:
>>>> On 11 Oct 2016, at 16:46, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
>> [snip]
>>>> -void stop_multi_file_filter(struct child_process *process)
>>>> +static void stop_multi_file_filter(struct child_process *process)
>>>
>>> Done! Do you have some kind of script to detect these things
>>> automatically or do you read the code that carefully?
>>
>> Heh, I'm _far_ too lazy to read the code that carefully. :-D
>>
>> A combination of 'make sparse' and a perl script (originally
>> posted to the list by Junio) find all of these for me.
> 
> Interesting. Do you have a link to this script?

I'm attaching _a_ version of the script (you would think that I would have it under version control; but no, I copy versions around the several machines/git.git repos I have ... :-P ).

> Does it generate false positives?

Hmm, well, you have to remember that 'make clean' sometimes doesn't make clean. Ever since the Makefile was changed to only remove $(OBJECTS), rather than *.o xdiff/*.o etc., you have to remember to 'make clean' _before_ you switch branches. Otherwise, you risk leaving some objects laying around. Since the script runs 'nm' on all objects it finds, any stale ones can cause problems. (Of course, I almost always forget, so I frequently have to manually check for and remove stale objects!)

Also, you have to interpret the results of the script. Just because it shows you an unused external, that doesn't make it an error. The way I use it, BTW, is to run it on a freshly built branch (saving to a file) and then compare the files from master->next->pu. I don't bother looking at the output on master (the 'stop list' in the script is _way_ out of date), just the diffs master->next and next->pu.

The current next->pu diff looks like:
    $ diff nsc psc
    0a1,2
    > attr.o	- attr_name_valid
    > attr.o	- invalid_attr_name_message
    17a20
    > convert.o	- stop_multi_file_filter
    34d36
    < quote.o	- sq_quotef
    43a46
    > sequencer.o	- append_new_todo
    50a54
    > trailer.o	- conf_head
    55a60,61
    > wt-status.o	- has_uncommitted_changes
    > wt-status.o	- has_unstaged_changes
    $

I have already sent mails about 'stop_multi_file_filter', 'append_new_todo' and 'conf_head'. The symbols in attr.o are part of the revamp of the attribute system that Junio and Stefan are working on, which is very much in flux. If they are still there when it looks to have firmed up, then I will take a look. The 'sq_quotef' symbol is an example of a useful function that was written without having an immediate user (actually, if memory serves correctly, it was originally used in the series which introduced it, but later modifications removed the caller). It now has a user. The wt-status.o symbols have been introduced by Johannes in his 'rebase -i' series (of series) and I am assuming that these symbols will find additional external callers in up-coming series.

> Maybe I can add `make sparse` to the TravisCI build?

I don't know how feasible that would be. Apparently, the version of sparse in most distribution repositories is so old that it spews so many spurious errors as to make it unusable. See, for example:

https://public-inbox.org/git/56CA5753.9030308@ramsayjones.plus.com/

So, you would need to build from source to get a usable version (I am currently running a version built with some local patches). If you were to build from the sparse repo's master branch, then it would work great on Linux, at least. I have no idea if it would work on mac OS X.

However, even then, a single sparse warning is issued on Linux (for the master branch, several others on pu):

        SP pack-revindex.c
    pack-revindex.c:65:23: warning: memset with byte count of 262144

This is a problem with sparse (the byte count limit used is hardcoded into sparse), but I haven't sent a patch upstream to fix it yet. (I've been looking at that warning for _many_ years, so don't hold your breath!)

ATB, Ramsay Jones

Jeff King· Oct 17, 2016, 02:18 UTC · re: Ramsay Jones · lore

Re: [PATCH] convert: mark a file-local symbol static

On Mon, Oct 17, 2016 at 02:37:58AM +0100, Ramsay Jones wrote:
Show 8 quoted lines
> Hmm, well, you have to remember that 'make clean' sometimes
> doesn't make clean. Ever since the Makefile was changed to only
> remove $(OBJECTS), rather than *.o xdiff/*.o etc., you have to
> remember to 'make clean' _before_ you switch branches. Otherwise,
> you risk leaving some objects laying around. Since the script
> runs 'nm' on all objects it finds, any stale ones can cause problems.
> (Of course, I almost always forget, so I frequently have to manually
> check for and remove stale objects!)

Gross. I would not be opposed to a Makefile rule that outputs the correct set of OBJECTS so this (or other) scripts could build on it.

IIRC, BSD make has an option to do this "make -V OBJECTS" or something, but I don't thnk there's an easy way to do so.

Or, since it seems to find useful results quite frequently, maybe it would be worth including the script inside git (and triggering it with an optional Makefile rule). It sounds like we'd need a way to annotate known false positives, but if it were in common use, it would be easier to get people to keep that list up to date.

-Peff
Johannes Schindelin· Oct 17, 2016, 09:04 UTC · re: Jeff King · lore

Re: [PATCH] convert: mark a file-local symbol static

Hi Ramsay & Peff,
On Sun, 16 Oct 2016, Jeff King wrote:
Show 13 quoted lines
> On Mon, Oct 17, 2016 at 02:37:58AM +0100, Ramsay Jones wrote:
> 
> > Hmm, well, you have to remember that 'make clean' sometimes doesn't
> > make clean. Ever since the Makefile was changed to only remove
> > $(OBJECTS), rather than *.o xdiff/*.o etc., you have to remember to
> > 'make clean' _before_ you switch branches. Otherwise, you risk leaving
> > some objects laying around. Since the script runs 'nm' on all objects
> > it finds, any stale ones can cause problems.  (Of course, I almost
> > always forget, so I frequently have to manually check for and remove
> > stale objects!)
> 
> Gross. I would not be opposed to a Makefile rule that outputs the
> correct set of OBJECTS so this (or other) scripts could build on it.

You could also use the method I use in Git for Windows to "extend" the Makefile:

-- snipsnap -- cat >dummy.mak <<EOF include Makefile

blub: $(OBJECTS)
	do-something-with $^
EOF
make -f dummy.mak blub
Jeff King· Oct 17, 2016, 09:37 UTC · re: Johannes Schindelin · lore

Re: [PATCH] convert: mark a file-local symbol static

On Mon, Oct 17, 2016 at 11:04:19AM +0200, Johannes Schindelin wrote:
Show 15 quoted lines
> > Gross. I would not be opposed to a Makefile rule that outputs the
> > correct set of OBJECTS so this (or other) scripts could build on it.
> 
> You could also use the method I use in Git for Windows to "extend" the
> Makefile:
> 
> -- snipsnap --
> cat >dummy.mak <<EOF
> include Makefile
> 
> blub: $(OBJECTS)
> 	do-something-with $^
> EOF
> 
> make -f dummy.mak blub
Hacky but clever. I like it.

In the particular case of git, I think I've cheated similarly before by putting things in config.mak, though of course an arbitrary script can't assume it can overwrite that file.

-Peff
Ramsay Jones· Oct 17, 2016, 17:21 UTC · re: Jeff King · lore

Re: [PATCH] convert: mark a file-local symbol static

On 17/10/16 10:37, Jeff King wrote:
Show 23 quoted lines
> On Mon, Oct 17, 2016 at 11:04:19AM +0200, Johannes Schindelin wrote:
> 
>>> Gross. I would not be opposed to a Makefile rule that outputs the
>>> correct set of OBJECTS so this (or other) scripts could build on it.
>>
>> You could also use the method I use in Git for Windows to "extend" the
>> Makefile:
>>
>> -- snipsnap --
>> cat >dummy.mak <<EOF
>> include Makefile
>>
>> blub: $(OBJECTS)
>> 	do-something-with $^
>> EOF
>>
>> make -f dummy.mak blub
> 
> Hacky but clever. I like it.
> 
> In the particular case of git, I think I've cheated similarly before by
> putting things in config.mak, though of course an arbitrary script can't
> assume it can overwrite that file.
Heh, I actually have the following in my config.mak already:
extra-clean: clean
	find . -iname '*.o' -exec rm {} \;

But for some reason I _always_ type 'make clean' and then, to top it off, I _always_ type the 'find' command by hand (I have no idea why) :-D

ATB, Ramsay Jones

Junio C Hamano· Oct 17, 2016, 20:07 UTC · re: Ramsay Jones · lore

Re: [PATCH] convert: mark a file-local symbol static

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
Show 8 quoted lines
> Heh, I actually have the following in my config.mak already:
>
> extra-clean: clean
> 	find . -iname '*.o' -exec rm {} \;
>
> But for some reason I _always_ type 'make clean' and then, to top
> it off, I _always_ type the 'find' command by hand (I have no idea
> why) :-D
"git clean -x" anybody?
René Scharfe· Oct 17, 2016, 20:48 UTC · re: Junio C Hamano · lore

Re: [PATCH] convert: mark a file-local symbol static

Am 17.10.2016 um 22:07 schrieb Junio C Hamano:
Show 12 quoted lines
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
>
>> Heh, I actually have the following in my config.mak already:
>>
>> extra-clean: clean
>> 	find . -iname '*.o' -exec rm {} \;
>>
>> But for some reason I _always_ type 'make clean' and then, to top
>> it off, I _always_ type the 'find' command by hand (I have no idea
>> why) :-D
>
> "git clean -x" anybody?
This removes config.mak as well.
René
Stefan Beller· Oct 17, 2016, 20:48 UTC · re: Junio C Hamano · lore

Re: [PATCH] convert: mark a file-local symbol static

On Mon, Oct 17, 2016 at 1:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 12 quoted lines
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
>
>> Heh, I actually have the following in my config.mak already:
>>
>> extra-clean: clean
>>       find . -iname '*.o' -exec rm {} \;
>>
>> But for some reason I _always_ type 'make clean' and then, to top
>> it off, I _always_ type the 'find' command by hand (I have no idea
>> why) :-D
>
> "git clean -x" anybody?

I only want to cleanup compiled stuff and such, not the precious unversioned text files I have laying around here. So I guess

  git clean -x -e:(attr:!binary)
would suffice, though. ;)
Ramsay Jones· Oct 17, 2016, 21:48 UTC · re: Junio C Hamano · lore

Re: [PATCH] convert: mark a file-local symbol static

On 17/10/16 21:07, Junio C Hamano wrote:
Show 12 quoted lines
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> 
>> Heh, I actually have the following in my config.mak already:
>>
>> extra-clean: clean
>> 	find . -iname '*.o' -exec rm {} \;
>>
>> But for some reason I _always_ type 'make clean' and then, to top
>> it off, I _always_ type the 'find' command by hand (I have no idea
>> why) :-D
> 
> "git clean -x" anybody?

I don't use 'git clean' because, on the very few occasions that I have tried to use it, it always deletes _far_ more than I thought it would or should. (particularly config.mak). Hmm, "git clean -X -- '*.o'" _might_ do what I want, but 'find' is so much easier ... :-D

ATB, Ramsay Jones

Ramsay Jones· Oct 17, 2016, 17:15 UTC · re: Jeff King · lore

Re: [PATCH] convert: mark a file-local symbol static

On 17/10/16 03:18, Jeff King wrote:
Show 16 quoted lines
> On Mon, Oct 17, 2016 at 02:37:58AM +0100, Ramsay Jones wrote:
> 
>> Hmm, well, you have to remember that 'make clean' sometimes
>> doesn't make clean. Ever since the Makefile was changed to only
>> remove $(OBJECTS), rather than *.o xdiff/*.o etc., you have to
>> remember to 'make clean' _before_ you switch branches. Otherwise,
>> you risk leaving some objects laying around. Since the script
>> runs 'nm' on all objects it finds, any stale ones can cause problems.
>> (Of course, I almost always forget, so I frequently have to manually
>> check for and remove stale objects!)
> 
> Gross. I would not be opposed to a Makefile rule that outputs the
> correct set of OBJECTS so this (or other) scripts could build on it.
> 
> IIRC, BSD make has an option to do this "make -V OBJECTS" or something,
> but I don't thnk there's an easy way to do so.

Hmm, I would go in the opposite direction and take a leaf out of Ævar's book (see commit bc548efe) and this one-liner:

Show changes to Makefile +1 −1
diff --git a/Makefile b/Makefile
index ee89c06..c08c25e 100644
--- a/Makefile
+++ b/Makefile
@@ -2506,7 +2506,7 @@ profile-clean:
 
 clean: profile-clean coverage-clean
        $(RM) *.res
-       $(RM) $(OBJECTS)
+       $(RM) $(addsuffix *.o,$(object_dirs))
        $(RM) $(LIB_FILE) $(XDIFF_LIB) $(VCSSVN_LIB)
        $(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X
        $(RM) $(TEST_PROGRAMS) $(NO_INSTALL)

This would actually solve my problem, but it actually isn't a
_complete_ solution. (Hint: think about what isn't in $(OBJECTS),
depending on the configuration). ;-)

> Or, since it seems to find useful results quite frequently, maybe it
> would be worth including the script inside git (and triggering it with
> an optional Makefile rule). It sounds like we'd need a way to annotate
> known false positives, but if it were in common use, it would be easier
> to get people to keep that list up to date.

Hmm, I suspect that wouldn't happen, which would reduce it usefulness
and ultimately lead to it not being used. (Updating the 'stop list' would
fast become a burden.)

I find it useful to flag these issues automatically, but I still need
to look at each symbol and decide what to do (you may not agree with
some of my choices either - take a look at the output on the master
branch!).

The way I use it, I effectively ignore the 'stop list' maintenance issues.

ATB,
Ramsay Jones

← back to recent threads