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] |
Hi David, thanks for the new patch. On Feb 27 17:13, David Allsopp wrote: > Corinna Vinschen wrote: > > On Feb 25 16:27, David Allsopp wrote: > > > This patch (below - I hope I have managed to format this email > > > correctly) alters the behaviour of dll_list::topsort to preserve the > > > order of dlopen'd units. > > > [...] > > > This patch is licensed under 2-clause BSD as per winsup/CONTRIBUTORS, > > > Copyright (c) 2017, MetaStack Solutions Ltd. Do you really want to make it (c) MetaStack? I'm asking because this way I can't add you personally as contributor to the CONTRIBUTORS file and you will have to continue to add per-patch copyright. The idea of the CONTRIBUTORS file was to claim BSD 2-clause for your first and all subsequent patches you provide to Cygwin, so you never have to think about the copyright stuff again. Your choice. > > - While you're at it, please reformat your patch so the line length > > is not longer than 80 chars. > > Done - sorry, I'd inferred a longer length from a few other longer lines! Yeah, the surrounding codes has a few minor formatting issues, in fact. > > - Last but not least. You add code to topsort so the loaded DLLs > > are handled first. The subsequent code is untouched. However, > > shouldn't the next loop then restrict calling populate_deps to the > > linked DLLs only, at least for performance? > > Oops :$ That's an artefact of the "story" of the patch's development. > As it happens, the first dlopen'd DLL would have been initialised in > the second loop, not the first, but the presence of two loops like > that was indeed mostly inefficient. I've kept the original one as a > "fast path" for the case of no dlopen'd DLLs, though I don't know if > that's a worthwhile optimisation. Well, interesting point. Basically your new code is a drop-in replacement, except for the fact that it always calls an extra cmalloc/cfree. However, this is only required if loaded_dlls > 0 so I think we may get away with removing the old loop with a simple tweak to your new one: dll** dlopen_deps = NULL; if (loaded_dlls > 0) dlopen_deps = (dll**) cmalloc (HEAP_2_DLL, maxdeps*sizeof (dll*)); while ((d = d->next)) { [...] } if (dlopen_deps) cfree (dlopen_deps); Do you want to tweak your patch accordingly? 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
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |