On (13/02/14 15:23), Michal Židek wrote:
On 02/13/2014 01:40 PM, Lukas Slebodnik wrote:
>On (13/02/14 10:55), Michal Židek wrote:
>>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 :)
>>
>
>Previously, I included two header files, but there was an objection
>@see
>https://lists.fedorahosted.org/pipermail/sssd-devel/2014-January/018015.html
>
Yes, I read the thread and I am fine with the current patch. It is
just that the current patch description looks to be a little misleading
to me.
The patch in this thread is no longer valid, because it is incorporated into
another patch set.
LS