On 01/17/2014 04:30 PM, Lukas Slebodnik wrote:
On (13/01/14 10:39), Dmitri Pal wrote:
> On 01/13/2014 09:54 AM, Lukas Slebodnik wrote:
>> On (13/01/14 09:07), Dmitri Pal wrote:
>>> On 01/13/2014 05:49 AM, Lukas Slebodnik wrote:
>>>> ehlo,
>>>>
>>>> Function ini_config_serialize was declared in the header file
ini_configobj.h,
>>>> but this header file was not included in implementation module
ini_serialize.c
>>> This is fine though I do not see a big reason to do this.
>>>
>>>> Functions print_config_parsing_errors, print_file_parsing_errors were
declared
>>>> in the header file ini_config.h, but this header file was not included
>>>> in implementation module ini_print.c
>>> ini_config.h is the old interface
>>> ini_configobj.h is the new interface
>> Yes,
>> but problem is that implementations of functions from old interface are in the
>> same module (ini_print.c) like implementations of functions from old interface.
>> If you do not like including both header files in one module, we should file a
>> ticket to split file ini_print.c. It is internal change and I hope easy task.
>>
>> But without this patch there will be some warnings:
>> CC ini_print.lo
>> ini/ini_print.c:327:6: warning: no previous prototype for
'print_file_parsing_errors' [-Wmissing-prototypes]
>> void print_file_parsing_errors(FILE *file,
>> ^
>> ini/ini_print.c:405:6: warning: no previous prototype for
'print_config_parsing_errors' [-Wmissing-prototypes]
>> void print_config_parsing_errors(FILE *file,
>> ^
>
> I took a look at these two functions. They are not used in the new
> interface.
> I suggest we just explicitly add declarations for these two functions to
> the top of ini_print.c instead of including the header file.
Done
> Splitting the module is a more labor intensive change. I would rather
> wait some time and just remove the old interface.
> Then split would not be needed.
>
>>> These headers should not be included into one module.
>>> They are mutually exclusive.
>>>
>>> Functions should probably be included into ini_configobj.h but I have
>>> not included them because they are already declared in the old interface.
>>> So may be we should move them from old to new rather than declare them
>>> twice?
>>>
New file is attached.
LS
The patch itself is OK. Just a (very) small nitpick. Could you change
the second part of patch description so that it is clear that you just
added the declarations (from the description it looks like you wanted to
include the header). I do not think anyone will actually read it and
yes, it is obvious from the patch what you did, but if you fix this it
will make me happy :)
Michal