[PATCH] Fix memory leak and add small optiomization

[Dan Nicholson]

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