Re: 330-gary-ltdl-vs-need-lib-prefix-unknown

[Gary V. Vaughan]

Hallo Ralf,

Thanks for the review :)

On Jun 24, 2007, at 7:27 AM, Ralf Wildenhues wrote:
> Review for the remaining stuff:
>
> [ tryall_dlopen ]
> > -    const lt_dlvtable *vtable = 0;
> > -    lt_dlloader *loader = 0;
> > +    lt_dlloader loader = lt_dlloader_next (0);
> > +    const lt_dlvtable *loader_vtable;
> >
> > -    while ((loader = (lt_dlloader *) lt_dlloader_next (loader)))
> > +    do
>
> This change tells me the internal typing system sucks.  If you at one
> time use a lt_dlloader and then at the next day use lt_dlloader* in
> a purely internal static function, then there is something wrong.

Hmmm... That does look weird.  Maybe it's a typo?  I'll check through
carefully before I commit.

> If we need to use pointers to void for hiding in external headers,  
> then
> so be it.  But most of the time I don't see why this:
>   struct lt_foo;  /* declaration only, no definition */
>   typedef struct lt_foo *lt_foo_handle;
>   int lt_foofunc (lt_foo_handle);
>
> in an exported header, should a problem.  (Of course where we know
> lt_foo will always be a struct.)
>
> I'm not asking to change this right now, although I am pretty  
> certain it
> should help.  What I'm asking is that we refrain from adding more of
> this nonsense.

ACK.

> Please add necessary casts to this patch so compilation with C++
> compiler is not broken again.

Sounds like we need a test that tries to compile libltdl with C++.   
Before
I submit a patch, I simply ensure that there are no regressions in the
testsuite on a couple of system types (usually darwin and Linux).  I  
don't
use C++ (or even know it particularly well), and will definitely  
forget to
run everything through a C++ compiler by hand after each patch I write.

> > @@ -1225,16 +1285,14 @@ try_dlopen (lt_dlhandle *phandle, const
> >          of libtool */
> >        int      installed = 1;
> >
> > -
> >        /* Now try to open the .la file.  If there is no directory  
> > name
> >           component, try to find it first in user_search_path and  
> > then other
> >           prescribed paths.  Otherwise (or in any case if the  
> > module was not
> >           yet found) try opening just the module name as passed.  */
> >        if (!dir)
> >         {
> > -         const char *search_path;
> > +         const char *search_path = user_search_path;
> >
> > -         search_path = user_search_path;
> >           if (search_path)
> >             file = find_file (user_search_path, base_name, &dir);
> >
> > @@ -1277,7 +1335,7 @@ try_dlopen (lt_dlhandle *phandle, const
> >        /* read the .la file */
> >        if (parse_dotla_file(file, &dlname, &libdir, &deplibs,
> >             &old_name, &installed) != 0)
> > -       errors++;
> > +       ++errors;
> >
> >        fclose (file);
> >
> > @@ -1347,7 +1405,7 @@ try_dlopen (lt_dlhandle *phandle, const
> >          Otherwise (or in any case if the module was not yet found) try
> >          opening just the module name as passed.  */
> >        if ((dir || (!find_handle (user_search_path, base_name,
> > -                                &newhandle, advise)
> > +                                &newhandle, advise)
> >                    && !find_handle (getenv (LTDL_SEARCHPATH_VAR), base_name,
> >                                     &newhandle, advise)
> >  #if defined(LT_MODULE_PATH_VAR)
> > @@ -1356,14 +1414,14 @@ try_dlopen (lt_dlhandle *phandle, const
> >  #endif
> >  #if defined(LT_DLSEARCH_PATH)
> >                    && !find_handle (sys_dlsearch_path, base_name,
> > -                                   &newhandle, advise)
> > +                                   &newhandle, advise)
> >  #endif
> >                    )))
> >         {
> > -          if (tryall_dlopen (&newhandle, filename, advise) != 0)
> > +         if (tryall_dlopen (&newhandle, filename, advise, 0) != 0)
> >              {
> >                newhandle = NULL;
> > -            }
> > +           }
> >         }
> >
> >        if (!newhandle)
>
> These hunks are all superfluous, except of course the one functional
> change to the tryall_dlopen call, for which one must look closely  
> to not
> overlook it.  Please separate functional changes from cleanup  
> changes in
> patch postings, that would make the former be much more easily  
> spottable.

That's because you asked me to run the file through 's,        ,^I,' !!

> Otherwise the changes look ok to me.  Have you done testing on  
> different
> systems?  I haven't, only GNU/Linux.  Thanks.

Darwin only so far.  It is too much work to run the testsuite through  
the
full gamut of hosts I have access to for every patch I submit, so I tend
to do that closer to alpha releases, and every now and again as time  
permits.

If I don't get to it beforehand, I'll go through the dozen or so  
architectures
I use as soon as the last few release blockers are closed.

Cheers,
        Gary
--
  ())_.              Email me: address@hidden
  ( '/           Read my blog: http://blog.azazil.net
  / )=         ...and my book: http://sources.redhat.com/autobook
`(_~)_ Join my AGLOCO Network: http://www.agloco.com/r/BBBS7912

PGP.sig
Description: This is a digitally signed message part