This is the mail archive of the cygwin-patches mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 1/4] dlopen: switch to new pathfinder class


On 09/02/2016 10:52 AM, Corinna Vinschen wrote:
> On Sep  2 10:05, Michael Haubenwallner wrote:
>> On 09/01/2016 04:03 PM, Corinna Vinschen wrote:
>>> On Sep  1 13:05, Michael Haubenwallner wrote:
>>>> On 08/31/2016 09:12 PM, Corinna Vinschen wrote:
>>>>> On Aug 31 20:07, Michael Haubenwallner wrote:
>>>>>> Instead of find_exec, without changing behaviour use new pathfinder
>>>>>> class with new allocator_interface around tmp_pathbuf and new vstrlist
>>>>>> class.
>>>>>> * pathfinder.h (pathfinder): New file.
>>>>>> * vstrlist.h (allocator_interface, allocated_type, vstrlist): New file.
>>>>>> * dlfcn.cc (dlopen): Avoid redundant GetModuleHandleExW with RTLD_NOLOAD
>>>>>> and RTLD_NODELETE.  Switch to new pathfinder class, using
>>>>>> (tmp_pathbuf_allocator): New class.
>>>>>> (get_full_path_of_dll): Drop.
>>>>>> [...]
>>>>>
>>>>> Just one nit here:
>>>>>
>>>>>> +/* Dumb allocator using memory from tmp_pathbuf.w_get ().
>>>>>> +
>>>>>> +   Does not reuse free'd memory areas.  Instead, memory
>>>>>> +   is released when the tmp_pathbuf goes out of scope.
>>>>>> +
>>>>>> +   ATTENTION: Requesting memory from an instance of tmp_pathbuf breaks
>>>>>> +   when another instance on a newer stack frame has provided memory. */
>>>>>> +class tmp_pathbuf_allocator
>>>>>> +  : public allocator_interface
>>>>>
>>>>> You didn't reply to
>>>>> https://cygwin.com/ml/cygwin-developers/2016-08/msg00013.html
>>>>> So, again, why didn't you simply integrate a tmp_pathbuf member into the
>>>>> pathfinder class, rather than having to create some additional allocator
>>>>> class?  I'm probably not the most diligent C++ hacker, but to me this
>>>>> additional allocator is a bit confusing.
>>>>
>>>> Sorry, seems I've failed to fully grasp your concerns firsthand in
>>>> https://cygwin.com/ml/cygwin-developers/2016-08/msg00016.html
>>>> Second try to answer:
>>>> https://cygwin.com/ml/cygwin-developers/2016-09/msg00000.html
>>>
>>> Ok, I see what you mean, but it doesn't make me really happy.
>>>
>>> I'm willing to take it for now but I'd rather see basenames being a
>>> member of pathfinder right from the start, so you just instantiate
>>> finder and call methods on it.
>>
>> The idea to build the basenames list before constructing pathfinder
>> is that members of the searchdirs list reserve space for the maxlen
>> of basenames:  This implies that the basenames list must not change
>> after the first searchdir was registered.
>>
>> To make sure this doesn't happen I prefer to not provide such an API
>> at all, rather than to check within some pathfinder::add_basename ()
>> method and abort if there is some searchdir registered already.
> 
> Yes, that sounds good.
> 
>>> Given that basenames is a member,
>>> you can do the allocator stuff completely inside the pathfinder class.
>>
>> Moving the allocator into pathfinder would work then, but still the
>> tmp_pathbuf instance to use has to be provided as reference.
> 
> Hmm, considering that a function calling your pathfinder *might*
> need a tmp_pathbuf for its own dubious purposes, this makes sense.
> That could be easily handled via the constructor I think:
> 
>   tmp_pathbuf tp;
>   pathfinder finder (tp);
> 
> Still, since I said I'm willing to take this code as is, do you want me
> to apply it this way for now or do you want to come up with the proposed
> changes first?

As I do prefer both pathfinder and vstrlist to not know about tmp_pathbuf
in particular but a generic memory provider only: Yes, please apply as is.

Thanks a lot!
/haubi/


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]