Sv, 2011-10-09 23:48 +0300, My Th rakstīja: > S , 2011-10-08 21:13 +0100, Chris Morley rakstīja: > > On 08/10/2011 20:02, My Th wrote: > > > It might be that the changes could be implemented in other place which > > > would shrink the patch size, but I don't know the OB code that well, so > > > I went for the most straightforward solution. > > > > Could a single line > > static int n = LoadAllPlugins(); > > in the function OBPlugin::PluginMap() replace all the other calls? > > The local static variable n would be initialized once on first use, but > > not subsequently used. > > > > I'm impressed that you took the trouble to understand the OBPlugin code. > > In the hybrid systems Cygwin and Mingw32 the plugins were originally not > > being loaded. Tim Vandermeersch made some changes which fixed this, but > > it would be helpful, if you are interested, in explaining how they work. > > (The OBPlugin code is a bit obscure; I can understand why macros are > > considered evil, http://www.parashift.com/c++-faq-lite ) > > The biggest problem with the macros in this case is that the code in > them is so badly formatted that reading it makes head hurt. And on top > of that one has to follow the subclassing of OBPlugin -> plugin types -> > plugin sub-types. > > OBPlugin::PluginMap() is the key point here. It has to be fully > populated before any lookup in that is done. > > Plugin map is a two level construct: > - First is a list of plugin types > - Second is a list of sub-types for each plugin type > > As I understand, the plugin map is populated by making instances of > specific plugin sub-types. Loading plugins makes also all the instances > of those plugin sub-types -> plugin type map gets populated. So the > function OBPlugin::LoadAllPlugins() has to be called before the lookup > in plugin map is done. > > Besides loading the dynamic libraries, OBPlugin::LoadAllPlugins() also > have a hook for making instances of plugin classes using information in > a text file plugindefines.txt. > > The problem with putting OBPlugin::LoadAllPlugins() inside of > OBPlugin::PluginMap() is that OBDefine class, which makes instances of > plugin classes given in the text file, itself is a plugin and it fails > to make those instances if plugin loading is put there. > > It is required to make an instance of OBDefine after all the plugins > have been loaded to ensure that it can make instances of all the classes > the data file is referring to. OB knows about all available plugins only > after all those instances are made. > > The code in my patch ensures that all of the plugins are properly loaded > and it doesn't touch platform specific code for loading dynamic > libraries (that is done by DLHandler). It also doesn't change any > exposed interface, so ABI should be fine AFAIK. > > Only issue is that I can't see a single entry point where to put > OBPlugin::LoadAllPlugins(), at least not for the current OB plugin > system. Maybe the solution is to make wrapper function for > OBPlugin::PluginMap() and OBPlugin::LoadAllPlugins(), and make other > functions call the wrapper, letting only sub-class constructors to call > OBPlugin::PluginMap() directly. Does it sound reasonable? I can make > such version of the patch if needed. > > > Reinis
Regardless if this goes in 2.3.2 or 2.4, in mean time it should be mentioned in the documentation for 2.3.1 that OBConversion constructor has to be called to load all plugins and to be able to access all available forcfields, formats, etc. It would save some frustration for people trying to use OB. It is not very nice to be forced to read the code of the library to understand how to use it and why something doesn't work as expected. Currently there are several such places. For example, if using PyBel, nothing needs to be done, because it already runs the constructor, but if using only openbabel Python module openbabel.OBConversion() needs to be called. Reinis ------------------------------------------------------------------------------ All the data continuously generated in your IT infrastructure contains a definitive record of customers, application performance, security threats, fraudulent activity and more. Splunk takes this data and makes sense of it. Business sense. IT sense. Common sense. http://p.sf.net/sfu/splunk-d2d-oct _______________________________________________ OpenBabel-Devel mailing list OpenBabel-Devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openbabel-devel
