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/