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
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()).
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?
In my test program I also add a check for plugin iterators. Attached
updated versions of the patch and test program.
Reinis
diff --git a/include/openbabel/obconversion.h b/include/openbabel/obconversion.h
index 77d3038..8a1b09c 100644
--- a/include/openbabel/obconversion.h
+++ b/include/openbabel/obconversion.h
@@ -32,7 +32,6 @@ GNU General Public License for more details.
#include <strings.h>
#endif
-#include <openbabel/dlhandler.h>
#include <openbabel/oberror.h>
#include <openbabel/format.h>
#include <openbabel/lineend.h>
@@ -345,7 +344,6 @@ protected:
// static FMapType& FormatsMIMEMap();///<contains MIME and pointer to all OBFormat classes
typedef std::map<std::string,int> OPAMapType;
static OPAMapType& OptionParamArray(Option_type typ);
- static int LoadFormatFiles();
bool OpenAndSetFormat(bool SetFormat, std::ifstream* is, std::stringstream* ss=NULL);
std::string InFilename;
@@ -372,7 +370,6 @@ protected:
typedef FilteringInputStreambuf< LineEndingExtractor > LErdbuf;
LErdbuf* pLineEndBuf;
- static int FormatFilesLoaded;
OBBase* pOb1;
std::streampos wInpos; ///<position in the input stream of the object being written
std::streampos rInpos; ///<position in the input stream of the object being read
diff --git a/include/openbabel/plugin.h b/include/openbabel/plugin.h
index 98bf06d..c744d01 100644
--- a/include/openbabel/plugin.h
+++ b/include/openbabel/plugin.h
@@ -123,6 +123,13 @@ public:
///Returns the map of the subtypes
virtual PluginMapType& GetMap() const =0;
+private:
+ ///Keep a record if all plugins have been loaded
+ static int AllPluginsLoaded;
+
+ ///Load all plugins (formats, fingerprints, forcefields etc.)
+ static void LoadAllPlugins();
+
protected:
///\brief Returns a reference to the map of the plugin types.
/// Is a function rather than a static member variable to avoid initialization problems.
diff --git a/src/obconversion.cpp b/src/obconversion.cpp
index 87c7b2e..47dee13 100644
--- a/src/obconversion.cpp
+++ b/src/obconversion.cpp
@@ -222,8 +222,6 @@ namespace OpenBabel {
@endcode
*/
- int OBConversion::FormatFilesLoaded = 0;
-
// OBFormat* OBConversion::pDefaultFormat=NULL;
OBConversion::OBConversion(istream* is, ostream* os) :
@@ -235,8 +233,6 @@ namespace OpenBabel {
{
pInStream=is;
pOutStream=os;
- if (FormatFilesLoaded == 0)
- FormatFilesLoaded = LoadFormatFiles();
//These options take a parameter
RegisterOptionParam("f", NULL, 1,GENOPTIONS);
@@ -314,52 +310,6 @@ namespace OpenBabel {
}
//////////////////////////////////////////////////////
- int OBConversion::LoadFormatFiles()
- {
- int count=0;
- // if(FormatFilesLoaded) return 0;
- // FormatFilesLoaded=true; //so will load files only once
-#if defined(USING_DYNAMIC_LIBS)
- //Depending on availablilty, look successively in
- //FORMATFILE_DIR, executable directory,or current directory
- string TargetDir;
-#ifdef FORMATFILE_DIR
- TargetDir="FORMATFILE_DIR";
-#endif
-
- DLHandler::getConvDirectory(TargetDir);
-
- vector<string> files;
- if(!DLHandler::findFiles(files,DLHandler::getFormatFilePattern(),TargetDir)) return 0;
-
- vector<string>::iterator itr;
- for(itr=files.begin();itr!=files.end();++itr)
- {
- if(DLHandler::openLib(*itr))
- count++;
- // Error handling is now handled by DLHandler itself
- // else
- // obErrorLog.ThrowError(__FUNCTION__, *itr + " did not load properly", obError);
- }
-#else
- count = 1; //avoid calling this function several times
-#endif //USING_DYNAMIC_LIBS
-
- //Make instances for plugin classes defined in the data file
- //This is hook for OBDefine, but does nothing if it is not loaded
- //or if plugindefines.txt is not found.
- OBPlugin* pdef = OBPlugin::GetPlugin("loaders","define");
- if(pdef)
- {
- static vector<string> vec(3);
- vec[1] = string("define");
- vec[2] = string("plugindefines.txt");
- pdef->MakeInstance(vec);
- }
- return count;
- }
-
- //////////////////////////////////////////////////////
/// Sets the formats from their ids, e g CML.
/// If inID is NULL, the input format is left unchanged. Similarly for outID
/// Returns true if both formats have been successfully set at sometime
diff --git a/src/ops/loader.cpp b/src/ops/loader.cpp
index 5e7b9ed..847b638 100644
--- a/src/ops/loader.cpp
+++ b/src/ops/loader.cpp
@@ -44,7 +44,7 @@ PLUGIN_CPP_FILE(OBLoader)
///Class which makes instances of plugin classes from information in text file.
///This allows the commandline and GUI interfaces to be extended without recompiling.
///The class is itself a plugin but needs a short piece of code as a hook in
-///OBConversion::LoadFormatFiles(). This does nothing if the plugin is not loaded.
+///OBPlugin::LoadAllPlugins(). This does nothing if the plugin is not loaded.
class OBDefine : public OBLoader
{
public:
@@ -147,7 +147,7 @@ private:
system of the existence of OBDefine. It cannot do the work of the plugin and
parse the datafile, because the plugins referred to there may not have been
loaded yet. Another instance with the same ID is made using MakeInstance() in
-OBConversion::LoadFormatFiles() after all the plugins are present.*/
+OBPlugin::LoadAllPlugins() after all the plugins are present.*/
OBDefine placeholderOBDefine;
//************************************************************
diff --git a/src/plugin.cpp b/src/plugin.cpp
index b221715..07ee39c 100644
--- a/src/plugin.cpp
+++ b/src/plugin.cpp
@@ -17,6 +17,7 @@ General Public License for more details.
***********************************************************************/
#include <openbabel/babelconfig.h>
+#include <openbabel/dlhandler.h>
#include <openbabel/plugin.h>
#include <iterator>
@@ -28,14 +29,73 @@ namespace OpenBabel
OBPlugin::PluginMapType& OBPlugin::GetTypeMap(const char* PluginID)
{
PluginMapType::iterator itr;
+
+ // Make sure the plugins are loaded
+ if (AllPluginsLoaded == 0) {
+ OBPlugin::LoadAllPlugins();
+ }
+
itr = PluginMap().find(PluginID);
if(itr!=PluginMap().end())
return itr->second->GetMap();
return PluginMap();//error: type not found; return plugins map
}
+int OBPlugin::AllPluginsLoaded = 0;
+
+void OBPlugin::LoadAllPlugins()
+{
+ int count = 0;
+
+#if defined(USING_DYNAMIC_LIBS)
+ // Depending on availability, look successively in
+ // FORMATFILE_DIR, executable directory or current directory
+ string TargetDir;
+
+#ifdef FORMATFILE_DIR
+ TargetDir="FORMATFILE_DIR";
+#endif
+
+ DLHandler::getConvDirectory(TargetDir);
+
+ vector<string> files;
+ if(!DLHandler::findFiles(files,DLHandler::getFormatFilePattern(),TargetDir)) {
+ return;
+ }
+
+ vector<string>::iterator itr;
+ for(itr=files.begin();itr!=files.end();++itr) {
+ if(DLHandler::openLib(*itr))
+ count++;
+ }
+#else
+ count = 1; // Avoid calling this function several times
+#endif //USING_DYNAMIC_LIBS
+
+ // Status have to be updated now
+ AllPluginsLoaded = count;
+
+ // Make instances for plugin classes defined in the data file.
+ // This is hook for OBDefine, but does nothing if it is not loaded
+ // or if plugindefines.txt is not found.
+ OBPlugin* pdef = OBPlugin::GetPlugin("loaders","define");
+ if(pdef) {
+ static vector<string> vec(3);
+ vec[1] = string("define");
+ vec[2] = string("plugindefines.txt");
+ pdef->MakeInstance(vec);
+ }
+
+ return;
+}
+
OBPlugin* OBPlugin::BaseFindType(PluginMapType& Map, const char* ID)
{
+ // Make sure the plugins are loaded
+ if (AllPluginsLoaded == 0) {
+ OBPlugin::LoadAllPlugins();
+ }
+
if(!ID || !*ID)
return NULL;
PluginMapType::iterator itr = Map.find(ID);
@@ -50,6 +110,11 @@ OBPlugin* OBPlugin::GetPlugin(const char* Type, const char* ID)
if(Type!=NULL)
return BaseFindType(GetTypeMap(Type), ID);
+ // Make sure the plugins are loaded
+ if (AllPluginsLoaded == 0) {
+ OBPlugin::LoadAllPlugins();
+ }
+
//When Type==NULL, search all types for matching ID and stop when found
PluginMapType::iterator itr;
for(itr=PluginMap().begin();itr!= PluginMap().end();++itr)
@@ -65,6 +130,12 @@ bool OBPlugin::ListAsVector(const char* PluginID, const char* param, vector<stri
{
PluginMapType::iterator itr;
bool ret=true;
+
+ // Make sure the plugins are loaded
+ if (AllPluginsLoaded == 0) {
+ LoadAllPlugins();
+ }
+
if(PluginID)
{
if(*PluginID!=0 && strcmp(PluginID, "plugins"))
#include <openbabel/forcefield.h>
#include <openbabel/obconversion.h>
#include <openbabel/plugin.h>
#include <iostream>
using namespace std;
int main(int argc,char **argv) {
cout << "Try finding a forcefield in blank state.." << endl;
if (OpenBabel::OBPlugin::Begin("forcefields") != OpenBabel::OBPlugin::End("forcefields")) {
cout << "A forcefield found" << endl;
} else {
cout << "Plugin iterators broken!" << endl;
}
if (OpenBabel::OBForceField::FindForceField("UFF")) {
cout << "UFF forcefield found" << endl;
} else {
cout << "Not found!" << endl;
}
cout << "And after calling OpenBabel::OBConversion().." << endl;
OpenBabel::OBConversion();
if (OpenBabel::OBForceField::FindForceField("UFF")) {
cout << "UFF forcefield found" << endl;
} else {
cout << "Not found!" << endl;
}
if (OpenBabel::OBPlugin::Begin("forcefields") != OpenBabel::OBPlugin::End("forcefields")) {
cout << "A forcefield found" << endl;
} else {
cout << "Plugin iterators broken!" << endl;
}
cout << "Available plugin types" << endl;
OpenBabel::OBPlugin::List("plugins");
cout << "Available fingerprints (MACCS is from plugindefines.txt)" << endl;
OpenBabel::OBPlugin::List("fingerprints");
return 0;
}
------------------------------------------------------------------------------
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
