[PATCH] Fix memory leak and add small optiomization

[Marco Diego Aurélio Mesquita]

Hi Dan!

Attached is a patch implementing the optimization the way you
described. For some reason, check-sort-order fails after this change.

Any tips on how to fix it?


On Wed, Aug 24, 2016 at 4:26 PM, Dan Nicholson <nicholson at endlessm.com> wrote:
> On Mon, Aug 22, 2016 at 6:30 PM, Marco Diego Aurélio Mesquita
> <marcodiegomesquita at gmail.com> wrote:
>> Hi Dan,
>>
>> Thanks for reviewing my patch. Some points:
>>
>> On Mon, Aug 22, 2016 at 7:06 PM, Dan Nicholson <nicholson at endlessm.com> wrote:
>>> 1. I'd prefer to keep the searching done the way it is where the
>>> packages hash table is searched rather than open coding the checks in
>>> has_required_packages. That duplicates too much of the logic in a way
>>> that's likely to break. In that sense, I'd prefer to skip the
>>> optimization where package_init isn't run immediately.
>>>
>>
>> I'd really like to test for the presence of each requested .pc file
>> before calling package_init. Is there a cleaner way to do it?
>
> OK, so thinking on it some more, here's my take.
>
> Doing the small optimization you have here to check the requested
> packages before doing the scanning doesn't seem like a big win since
> in the common case (you have the package) you end up doing the
> scanning anyway.
>
> What would be a good optimization would be to change pkg-config so
> that it only does the scanning in the --list-all case. In other words,
> add packages to the hash table as needed instead of always filling it
> immediately. Here's a strawman approach to that.
>
> 1. Factor out the code from scan_dir that actually adds a package to
> the locations hashtable based on finding a .pc file. Something like
> add_package(path).
>
> 2. Change internal_get_package so that if location is NULL, search for
> the pc file like scan_dir does and add it to the locations hashtable.
> If it's still not found, return NULL like it does now.
>
> 3. Remove the scan_dir call in package_init. Add a helper function to
> call scan_dir and use that in main when --list-all is requested.
>
> There's some tricky code with scanned_dir_count in scan_dir that would
> need to continue working in the case where scan_dir was not called.
> The path_positions hash table needs to keep working correctly.
>
>>> 2. The leak can be fixed by simply calling g_list_free at the end of
>>> process_package_args, right?
>>>
>>
>> AFAICS yes.
>
> I would split this out to a separate patch if you still want to fix that.
>
> --
> Dan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pkg-config-optimization.patch
Type: text/x-patch
Size: 5868 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/pkg-config/attachments/20160824/8efc3f52/attachment.bin>