# static-check hits 2 messages from 2021-12-04 to 2021-12-04. Participants: Ramsay Jones, Taylor Blau. Thread: https://gitlist.dev/t/57032 ## Ramsay Jones, 2021-12-04 14:59 Subject: static-check hits Message-ID: URL: https://gitlist.dev/e/ec008905-5e04-944d-f164-d52a622f0531%40ramsayjones.plus.com ``` Hi Taylor, Just a quick note about new hits from my 'static-check.pl' script caused by the 'tb/cruft-packs' branch. This script notes any symbols that are not referenced outside the defining compilation unit. (So they could be declared static in that compilation unit). Comparing the current 'next' and 'seen' branches: $ diff nsc ssc ... 62a63,64 > pack-mtimes.o - pack_has_mtimes > packfile.o - close_pack_mtimes ... $ This is not necessarily a problem, of course, if you have patches/plans to add callers in the future (or that they simply 'round out' an API). I haven't looked (so can't comment), beyond: $ git grep -n pack_has_mtimes pack-mtimes.c:14:int pack_has_mtimes(struct packed_git *p) pack-mtimes.h:11:int pack_has_mtimes(struct packed_git *p); $ git grep -n close_pack_mtimes packfile.c:336:void close_pack_mtimes(struct packed_git *p) { packfile.c:350: close_pack_mtimes(p); packfile.h:94:void close_pack_mtimes(struct packed_git *p); $ Note that 'pack_has_mtimes()' has no callers at all. Also, the function definition of 'close_pack_mtimes()' has the opening { of the body on the function header line, rather than by itself on the following line. Just an FYI. ATB, Ramsay Jones ``` ## Taylor Blau, 2021-12-04 22:47 Subject: Re: static-check hits Message-ID: URL: https://gitlist.dev/e/YavwA6WFB3wyO76d%40nand.local In-Reply-To: ``` On Sat, Dec 04, 2021 at 02:59:15PM +0000, Ramsay Jones wrote: > Hi Taylor, > > Just a quick note about new hits from my 'static-check.pl' script > caused by the 'tb/cruft-packs' branch. This script notes any symbols > that are not referenced outside the defining compilation unit. > (So they could be declared static in that compilation unit). > Comparing the current 'next' and 'seen' branches: > > $ diff nsc ssc > ... > 62a63,64 > > pack-mtimes.o - pack_has_mtimes > > packfile.o - close_pack_mtimes > ... > $ > > This is not necessarily a problem, of course, if you have patches/plans > to add callers in the future (or that they simply 'round out' an API). > I haven't looked (so can't comment), beyond: Thanks very much for pointing both of these out. Removing pack_has_mtimes() entirely is fine with me. I was surprised that it was unused, since I thought the code setting `is_cruft = 1` in `add_packed_git()` would have been a potential caller, but that spot just constructs the path itself and checks the result access()-ing it. Similarly on close_pack_mtimes(): that was definitely intended to round out the API (along with close_pack_revindex()), but isn't necessary outside of packfile.c's compilation unit. We could probably apply the same treatment to close_pack_revindex(), but I'll pursue that as a separate matter. > Also, the function definition of 'close_pack_mtimes()' has the opening > { of the body on the function header line, rather than by itself on > the following line. Copied over from close_pack_revindex(), but fixed. Thanks! Thanks, Taylor ```