crfsuite icon indicating copy to clipboard operation
crfsuite copied to clipboard

missing inline

Open unnonouno opened this issue 13 years ago • 3 comments

In crfsuite.hpp, all definition of member functions such as Trainer::Trainer are not inline functions. So, when multiple source files include crfsuite.hpp, the c++ compiler reports duplicated symbols of these member functions. Please add "inline" to all the member functions defined in the header like this:

@@ -44,6 +44,7 @@
 namespace CRFSuite
 {

+inline
 Trainer::Trainer()
 {
     data = new crfsuite_data_t;

Thanks.

unnonouno avatar Jan 18 '12 12:01 unnonouno

Thank you for the comment.

The intention why I created two header files was that:

  1. "crfsuite_api.hpp" can be included multiple times where necessary, but "crfsuite.hpp" must be included only once
  2. "crfsuite.hpp" has a dependency to C API (crfsuite.h), whereas "crfsuite_api.hpp" has no external dependency (portable). If necessary, we can build a standalone pre-compiled C++ library (only with a single header file "crfsuite_api.hpp") that does not depend to the C header and library of libcrfsuite if necessary.

I should have named "crfsuite.hpp" as "crfsuite_impl.hpp" to clarify this role (and no mention about this role in the documentation :P )

At the same time, it is harmless to have "inline" to every member function in "crfsuite.hpp". I will commit the changes to the HEAD. BTW, I think "inline" should be "static inline" to achieve the goal.

chokkan avatar Jan 19 '12 03:01 chokkan

My mistake: we cannot specify "static inline" in "crfsuite.hpp". I'm confused with the usage of "static" keyword inside and outside of a class.

chokkan avatar Jan 19 '12 03:01 chokkan

I understood your intention. And, I correctly compiled my program with "crfsuite_api.hpp" and a cpp file that only includes "crfsuite.hpp". I think the standalone C++ library is preferable. And, in this case, maybe "inline" is not necessary because I can't link inline methods defined in the object file. I think inline methods can't be referred from other object files.

I should have named "crfsuite.hpp" as "crfsuite_impl.hpp" to clarify this role (and no mention about this role in the documentation :P )

It's a good idea. The role of these files are clear.

unnonouno avatar Jan 24 '12 12:01 unnonouno