[pacman-dev] [PATCH v3 2/3] util-common.h: Add ARRAYSIZE macro

[Allan McRae]

On 21/10/15 17:55, Pierre Neidhardt wrote:
> On 15-10-21 17:28:11, Allan McRae wrote:
>> On 21/10/15 17:15, Pierre Neidhardt wrote:
>>> On 15-10-21 14:49:21, Allan McRae wrote:
>>>> On 20/10/15 23:30, Pierre Neidhardt wrote:
>>>>> Signed-off-by: Pierre Neidhardt <ambrevar at gmail.com>
>>>>> ---
>>>>>  src/common/util-common.h | 2 ++
>>>>>  1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/src/common/util-common.h b/src/common/util-common.h
>>>>> index a2093be..373db19 100644
>>>>> --- a/src/common/util-common.h
>>>>> +++ b/src/common/util-common.h
>>>>> @@ -34,6 +34,8 @@ char *safe_fgets(char *s, int size, FILE *stream);
>>>>>  char *strndup(const char *s, size_t n);
>>>>>  #endif
>>>>>  
>>>>> +#define ARRAYSIZE(a) (sizeof (a) / sizeof (a[0]))
>>>>> +
>>>>>  #endif /* _PM_UTIL_COMMON_H */
>>>>>  
>>>>
>>>> This is needed nowhere in libalpm, so put this in src/pacman/util.c at
>>>> the top of the file.  And you might as well use it in the three places
>>>> that could use it in pacman while you are at it.
>>>>
>>>> Allan
>>>
>>> How do you define the appropriate spots where it can be used? I believe alpm
>>> could use the macro in many places as well. For instance, both pacman and alpm
>>> have numerous PATH_MAX referencing the size of an array (beside its
>>> initialization of course).
>>
>> PATH_MAX is used all over the place for storing paths.  They usually get
>> filled with a snprintf result and then have the length stored.  I don't
>> see a use for this define there.
> 
> PATH_MAX is not a great example as it is fairly idiomatic. But you can use
> ARRAYSIZE for any string manipulation, e.g.:
> 
> 	snprintf(s, ARRAYSIZE(s), ...
> 
> This way you can choose to restrict the explicit 'sizeof' calls to types. As was
> originally suggested in HACKING.
> 
> It's a simple matter of style consistency, and since this was not done before I
> don't think we should change it.

So we are both saying it should not get used in string manipulation?
Which is good because I would not accept changes to that...

>>> I endorse the use of ARRAYSIZE, but I must say it is quite inconsistent with
>>> what has been done until now. Suggestions:
>>>
>>> * We replace every static array size reference with a call to the macro. Lots of
>>> work and error prone.
>>
>> I can see use doing the size of calculation in three places:
>>
>> $ git grep sizeof | grep "\["
>> src/pacman/pacman.c: for(i = 0; i < sizeof(signals) /
>> sizeof(signals[0]); i++) {
>> src/pacman/sync.c: for(j = 0; j < sizeof(glob_skips) /
>> sizeof(glob_skips[0]); j++) {
>> src/pacman/util.c: static const int unitcount = sizeof(labels) /
>> sizeof(labels[0]);
> 
> Wrong regex:
> 
> 	$ git grep "sizeof *([^)]*) */"
> 	lib/libalpm/pkghash.c:	loopsize = sizeof(prime_list) / sizeof(*prime_list);
> 	src/pacman/pacman.c:	for(i = 0; i < sizeof(signals) / sizeof(signals[0]); i++) {
> 	src/pacman/sync.c:				for(j = 0; j < sizeof(glob_skips) / sizeof(glob_skips[0]); j++) {
> 	src/pacman/util.c:	static const int unitcount = sizeof(labels) / sizeof(labels[0]);
> 
> So yes, alpm has *one* use for it! :D
> 

OK - keep it in util-common then.   Will you submit a patch with those
changed to use ARRAYSIZE?  Or should I just patchcount++ myself?

A