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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
