implementation of minimal plugin system based on runtime polymorphism + factories #1
Conversation
JohanMabille
left a comment
JohanMabille
left a comment
There was a problem hiding this comment.
Really nice stuff, I really like the removal of the polymorphism for shared library implementations. Global remarks regarding the coding conventions:
- opening curly braces should be on a new line
- classes declarations should not contain any method implementation. I know it can be cumbersome to define the functions after, but it makes it easier to read the public API in the future.
- public/protected/private accessors should be at the same indentation level as classes declarations.
We can setup a pre-commit hook and some clang configuration to automatically format the code (but obviously that won't move the definitions out of the classes).
We can handle the "pure C init plugin functions" in a dedicated PR (if we decide it is worth doing so).
include/xplugin/xplugin_config.hpp
Outdated
| * The full license is in the file LICENSE, distributed with this software. * | ||
| ****************************************************************************/ | ||
|
|
||
| #pragma once |
There was a problem hiding this comment.
This is non standard directive and should be avoided.
There was a problem hiding this comment.
do you know any actually used compiler not supporting it? Fine with using include guards, but I think one can save assume that modern compilers support #pragma once
include/xplugin/xplugin_registry.hpp
Outdated
| @@ -0,0 +1,100 @@ | |||
| #pragma once | |||
There was a problem hiding this comment.
Same remark here, prefer include guards over non standard #pragma once
include/xplugin/xplugin_registry.hpp
Outdated
| } | ||
| else | ||
| { | ||
| xunix_shared_library library(find_res->second); |
include/xplugin/xplugin_registry.hpp
Outdated
| class xplugin_registry{ | ||
|
|
||
| public: | ||
| using create_plugin_factory_type = FACTORY * (*)(); |
There was a problem hiding this comment.
It could be useful to create a type alias on FACTORY
| * The full license is in the file LICENSE, distributed with this software. * | ||
| ****************************************************************************/ | ||
|
|
||
| #pragma once |
There was a problem hiding this comment.
This is non standard directive and should be avoided.
DerThorsten
changed the title
|
whoever merges this, pls do "Squash and merge" |
|
Bravo! |
SylvainCorlay
dismissed
JohanMabille’s stale review
I think the outstanding pragma once can be removed later.
| { | ||
| public: | ||
| using base_type = BASE_TYPE; | ||
| virtual ~xfactory_base() |
| using concrete_type = CONCRETE_TYPE; | ||
| using base_type = BASE_TYPE; | ||
| using factory_base_type = xfactory_base<BASE_TYPE, ARGS...>; | ||
| virtual ~xfactory() |
| namespace xp | ||
| { | ||
|
|
||
| template <class FACTORY_BASE> |
There was a problem hiding this comment.
Formatting: everything in the namespace should be indented
| template <class FACTORY_BASE> | ||
| class xplugin_registry | ||
| { | ||
| public: |
There was a problem hiding this comment.
Formatting: indent should be 4 spaces
| std::string name = entry.path().stem().string(); | ||
|
|
||
| // remove prefix | ||
| if (name.substr(0, prefix.size()) == prefix) |
There was a problem hiding this comment.
I think we should keep libraries without prefix in their path
There was a problem hiding this comment.
this is not for the path. this is for the name / key in the map.
For a lib at some/path/libfoobar.so we use foobar as name/key and some/path/libfoobar.so as value path/value.
We cannot keep the prefix in the name/ key, since this would give different name/keys on windows and UNIX.
Ie:
#if __unix__
auto foobar_factory = registry.create_factory("libfoobar")
#else // win
auto foobar_factory = registry.create_factory("foobar")
#endif while when we remove the prefix we can always use
auto foobar_factory = registry.create_factory("foobar")
|
@JohanMabille I addressed your comments in #2 |
3 participants
Todos: