Hi Gary,
* Gary V. Vaughan wrote on Mon, Nov 30, 2009 at 06:29:54PM CET:
> On 29 Nov 2009, at 16:27, Ralf Wildenhues wrote:
> > - slist_remove should IMVHO return an SList *, because otherwise there
> > is no way to avoid a memory leak. APIs that force memleaks are bad.
>
> I don't understand that assertion; where is the memory leak in the
> following code?
There is none. In your example, the callback, thus also slist_remove,
pass back an SList* already, which is good. But the API also allows
the callback to return a pointer to the userdata only (at least that's
how I understand it). That would still work alright with slist_find,
but with slist_remove, it necessarily causes a memory leak of the SList
structure pertaining to the data returned. That's the case where the
user code "forgets" to use slist_unbox, namely because the item it gets
is already unboxed, and the box already leaked.
> I think the boxing/unboxing is a very elegant way to provide for clear
> separation of concerns where the SList implementation handles the memory
> for the list chaining wrapper structures (the boxes), where the client
> code handles just the memory for the data being chained (a list of
> dlloaders in this case), while also avoiding hand writing the glue code
> to add the next pointers into structures that will be fed to SList.
I'm not sure I agree that this is elegant, but maybe I just
misunderstand the code.
But all of this is so unimportant to the libltdl code quality that I
prefer we just keep the code we have, and *not* start introducing more
new code with more new bugs here.
> Anyway, if you are proposing that SListCallback functions passed to
> slist_remove should always return SList *, then that does seem like a
> worthy addition to me.
No. I don't propose that. I only propose it for slist_remove, because
that's where otherwise, a memleak is inevitable. That's just what my
patch does, by letting slist_remove return an SList *.
> > (Apart from that, I've never really understood the difference between
> > boxed and unboxed stuff, basically you have to have a SList header in
> > order to put something in a list, period. But I didn't want to mangle
> > the code beyond bug fixing really.)
>
>
> SList is designed to manage two types of lists:
>
> (i) things that were specifically designed to be chained, in which
> case the first field of the structures to be chained has to be
> 'next':
>
> struct notboxed {
> struct notboxed *next;
> <whatever real data needs to be stored in the element>
> };
>
> (ii) things that were not designed to be chained, but that we want
> to handle lists of in any case. In this case, the SList api
> allows wrapping each item in an chainable element (boxing the
> item) and getting the original back out again (unboxing it).
>
> SList *open_filehandles = 0;
> ...
> FILE *fh = fopen ("/home/gary/input", "r");
>
> open_filehandles = slist_cons (slist_box (fh), open_filehandles);
> ...
> fclose (slist_unbox ((SList *) slist_remove (&open_filehandles,
> match_cb, matchdata)));
>
> otherwise, we'd need to build our own wrapper struct to contain
> the chain pointer and the FILE pointer.
I don't believe you that it was really designed to do (i). If it were,
then there were some code using it as such, which I would like to ask
you to submit as testsuite addition, so we can see whether, and *how*
it works at all, and the coverage may keep us from breaking it. I don't
believe that it works. Just like the sorting functions did not work.
If (i) does work, then likely my patch is broken after all; so I guess I
will have to wait with applying.
> > So I have three patches I would like to commit as part of this.
> > The first one adds the test and fixes slist.c,
>
> I recommend adding a new callback with SList* return type as I said
> above.
>
> > the second one is only
> > stylistic and uses NULL instead of 0 throughout slist.c,
>
> IIRC, it was trying to build libltdl with some old C++ compiler (maybe
> Solaris 7 or so, but I don't recall clearly) which complained about the
> use of NULL which drove me to using '0' in slist.c. I googled for
> NULL vs 0, and it seems there it has become a religious issue.
Oh well. I'm not attached to the NULL patch.
> I'm not entirely comfortable with this change while we tout the feature
> of being able to build libltdl with a C++ compiler. I'd like to see
> that C++ builds will still work correctly with explicit NULLs before we
> settle on this patch. But, if you're keen to commit but don't have time
> to check, then I'll probably trip over it at some point for as long as we
> have the C++ build requirement in HACKING.
Well I built it with g++ 4.2.4.
> > and the third
> > one changes our public APIs lt_dlloader_remove and lt_dlloader_find to
> > accept `const char *' arguments instead of `char *': that's what fits
> > the purpose best, and what we document in the manual. I'm not quite
> > sure whether the last one constitutes a compatible API change or only
> > a revision change, so versioning bump is still missing.
>
> Implicit 'char *' to 'const char *' ought to work seamlessly IIUC, so
> I'm not sure the version needs to change at all...
OK, thanks. Well, REVISION needs a bump before the next release of
course.
Cheers,
Ralf
