On 18/10/2011 20:18, My Th wrote: > O , 2011-10-18 17:54 +0100, Chris Morley rakstīja: >> On 12/10/2011 00:15, My Th wrote: >>> 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. >> >> The attached patch (against the current trunk) is a modification of your >> proposal, with LoadAllPlugins() called only during the initialization of >> a local static variable in OBPlugin::PluginMap(). This saves several >> other calls and a boolean. The initialization occurs at the first use of >> any plugin. OBDefine seems to work ok (see revised test program) but the >> global variable OBLocale was being called before it was initialized (the >> well-known static initialization problem). This was fixed by replacing >> the global variable by a local one in the OBDefine constructor, which I >> think should not affect its operation. >> >> This works with Visual Studio 8 on Windows 7 (all I have), but it would >> be nice to know know it worked on some other systems before committing it. >> >> Chris > > Hi! > > I have problems with your version of the patch. I'm using Gentoo Linux > with gcc 4.6.1. > > Running the test program I get this: > Try finding a forcefield in blank state.. > terminate called after throwing an instance of > '__gnu_cxx::recursive_init_error' > what(): std::exception > Aborted > It seems Visual C++ and gcc treat this differently, presumably Visual C++ has set the internal 'initialized' flag on the variable n before LoadAllPlugins() has returned, while gcc does it afterwards. PluginMap() is called when there is any use of any plugin and I see the first use is when OpGenAlias in the API code is made. The recursive passes I suspect are all the other plugins being made when LoadAllPlugins() runs.
> I think the reason for this exception is the same as I mention for why > LoadAllPlugins() can't be called from PluginMap() - the hook to > OBDefine. > > Also there is no point making the function LoadAllPlugins() to return > int if its not used. As it is now LoadAllPlugins() is called every time > the PluginMap() is accessed. In my version I had an if statement to > ensure that it is called only once and the status AllPluginsLoaded was > updated inside the function just before the hook for OBDefine (which > also accesses PluginMap()). In the line static int n = LoadAllPlugins(); in PluginMap(), LoadAllPlugins should be called only once when the local static variable n is initialized on the first pass. This certainly happens for me (using the debugger); does it not for you? > > I can't comment right now on the issue with OBLocale since the patch in > this form is clearly not working for me. I'm surprised it works on Win. > > I think the move of LoadAllPlugins() to private status is very > appropriate, so I'm changing that also in my version. > > Another question I have is how to handle openbabel/dlhandler.h include? > Is it really necessary to have it in both places - plugin.h and > plugin.cpp? I think one of them is enough. Probably tough it should go > in plugin.cpp since there is the definition of LoadAllPlugins() which > uses it. Am I correct? Yes, I meant to move it, but instead copied it. On second thoughts, I think it is better in plugin.h otherwise it needs to be included in a few other places in OB. Better to leave pluhin.h as the top level inclusive header for this area. > > In my test program I also add a check for plugin iterators. Attached > updated versions of the patch and test program. > With gcc not playing ball, I've committed your patch, which seems to work fine for me, to trunk. Chris > 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
