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

[Ramsay Jones <ramsay@ramsayjones.plus.com>]

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



On 16/10/16 01:15, Lars Schneider wrote:
>> 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



[-- Attachment #2: static-check.pl --]
[-- Type: application/x-perl, Size: 1333 bytes --]