[pacman-dev] [PATCH 1/2] Fix warnings not usually emitted (CFLAGS not in configure.ac)

[Dan McGee]

On Wed, Sep 15, 2010 at 8:07 AM, Allan McRae <allan at archlinux.org> wrote:
> On 13/09/10 00:01, Sebastian Nowicki wrote:
>>
>> -Wbad-function-cast:
>>        * casting long int to pmpkgreason_t unecessarily
>>        * casting pmpkgreason_t to long unecessarily
>>        * casting off_t (signed integral) to float before division
>>          unecessarily
>>
>> -Wshadow:
>>        * shadowing "handle" variables renamed to "new_handle" and
>>          "local_handle" shadowing "filestr" renamed to "pkgfilestr"
>>        * shadowing "remove" renamed to "remove_pkgs"
>>        * shadowing "sync" renamed to "syncpkg"
>>        * Removed redundant declarations
>>        * shadowing "prefix" renamed to "entry_prefix"
>>        * shadowing "pipe" renamed to "pipe_handle"
>>
>> -Wconversion:
>>        * explicitely cast nread to size_t from ssize_t in dload.c
>>          (guaranteed not to be negative at this point)
>>        * use size_t, as opposed to int, for string length and byte sizes
>>        * use signed int in alpm_list_count as advertised by API (should
>>          probably be changed to usngiend int or size_t, but requires an
>> API
>>          change)
>>
>> Signed-off-by: Sebastian Nowicki<sebnow at gmail.com>
>
> I tried having a look at this but kept getting lost in what part of the
> patch fixed what warnings.  e.g. I can not find the casting off_t to float
> change for -Wbad-function-cast.
>
> I think it would be much better to split these up a bit more.  Splitting by
> error type would be fine for me.

+1; if you could do one patch for each warning that would be awesome.
I know it is a bit more work but it makes it a lot easier to see what
you've done (as well as prevent it from sneaking in again in the
future).

>
>> ---
>> Some of these warnings are quite pedantic and wouldn't really cause
>> issues, so they might not be desired.
>>
>> The change in alpm_list_count is backwards in my opinion - the API
>> should be changed to use unsigned int (or size_t). I didn't want to
>> change the API though.
>
> I can not see why this ever returned a signed int.

This should really be size_t, just like strlen(), etc. If we want to
change it in a major version release, we haven't shied away from it in
the past, so I'd be for it. In most cases it wouldn't break code or
built applications anyway.

-Dan