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
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
Index: include/openbabel/obconversion.h
===================================================================
--- include/openbabel/obconversion.h (revision 4642)
+++ include/openbabel/obconversion.h (working copy)
@@ -32,7 +32,6 @@
#include <strings.h>
#endif
-#include <openbabel/dlhandler.h>
#include <openbabel/oberror.h>
#include <openbabel/format.h>
#include <openbabel/lineend.h>
@@ -345,7 +344,6 @@
// 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 @@
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
Index: include/openbabel/plugin.h
===================================================================
--- include/openbabel/plugin.h (revision 4644)
+++ include/openbabel/plugin.h (working copy)
@@ -20,6 +20,7 @@
#define OB_PLUGIN_H
#include <openbabel/babelconfig.h>
+#include <openbabel/dlhandler.h>
#include <string>
#include <iostream>
#include <vector>
@@ -123,12 +124,17 @@
///Returns the map of the subtypes
virtual PluginMapType& GetMap() const =0;
+private:
+ ///Load all plugins (formats, fingerprints, forcefields etc.)
+ static int 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.
static PluginMapType& PluginMap()
{
static PluginMapType m;
+ static int n = LoadAllPlugins();
return m;
}
Index: src/obconversion.cpp
===================================================================
--- src/obconversion.cpp (revision 4642)
+++ src/obconversion.cpp (working copy)
@@ -222,8 +222,6 @@
@endcode
*/
- int OBConversion::FormatFilesLoaded = 0;
-
// OBFormat* OBConversion::pDefaultFormat=NULL;
OBConversion::OBConversion(istream* is, ostream* os) :
@@ -235,8 +233,6 @@
{
pInStream=is;
pOutStream=os;
- if (FormatFilesLoaded == 0)
- FormatFilesLoaded = LoadFormatFiles();
//These options take a parameter
RegisterOptionParam("f", NULL, 1,GENOPTIONS);
@@ -314,52 +310,6 @@
}
//////////////////////////////////////////////////////
- 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
Index: src/ops/loader.cpp
===================================================================
--- src/ops/loader.cpp (revision 4642)
+++ src/ops/loader.cpp (working copy)
@@ -44,7 +44,7 @@
///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:
@@ -65,7 +65,9 @@
}
// Set the locale for number parsing to avoid locale issues: PR#1785463
- obLocale.SetLocale();
+ // The global instance may not have been made if this is called during
initialization of global vars
+ OBLocale localLocale;
+ localLocale.SetLocale();
string ln;
while(ifs) //read entries for multiple objects
@@ -102,7 +104,7 @@
// return the locale to the original one
- obLocale.RestoreLocale();
+ localLocale.RestoreLocale();
}
@@ -147,7 +149,7 @@
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;
//************************************************************
Index: src/plugin.cpp
===================================================================
--- src/plugin.cpp (revision 4642)
+++ src/plugin.cpp (working copy)
@@ -17,6 +17,7 @@
***********************************************************************/
#include <openbabel/babelconfig.h>
+#include <openbabel/dlhandler.h>
#include <openbabel/plugin.h>
#include <iterator>
@@ -34,6 +35,49 @@
return PluginMap();//error: type not found; return plugins map
}
+int 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 0;
+ }
+
+ 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
+
+ // 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;
+}
+
OBPlugin* OBPlugin::BaseFindType(PluginMapType& Map, const char* ID)
{
if(!ID || !*ID)
#include <openbabel/forcefield.h>
#include <openbabel/obconversion.h>
#include <iostream>
using namespace std;
int main(int argc,char **argv) {
cout << "Try finding a forcefield in blank state.." << 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;
}
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
