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

[Corinna Vinschen <corinna-cygwin at cygwin dot com>]

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?


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

Attachment: signature.asc
Description: PGP signature