dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> mgallien wrote in extractor.cpp:34
> If the Extractor is built using the move constructor, the other instance will
> have a null d pointer. As far as I know, this is standard practice.
You are correct. However, to improve readability, move (haha) the
implementation of the move constructor to be with the default constructor, i.e.
above this freeExtractorPlugin method. This will make it clearer that there are
multiple ctors, when reading top to bottom.
> extractor.cpp:36
> + if (d->m_pluginLoader.isLoaded()) {
> + d->m_pluginLoader.unload();
> + } else {
Are you sure you want to call unload? We've had many many problems in the past
with unloading plugins.
Anything that still refers to a static object from the plugin (e.g. QMetaObject
registration, or anything else) will crash.
> extractor.h:36
> public:
> + Extractor(Extractor &&other);
> +
add `noexcept`
> extractor.h:38
> +
> + void operator =(Extractor &&other);
> +
The proper syntax for a move constructor is
` Extractor& operator=(Extractor &&other) noexcept`
> extractorcollection.cpp:111
> + Extractor newExtractor;
> + newExtractor.d->m_pluginLoader.setFileName(pluginPath);
>
This lacks encapsulation. Add a setPluginFileName method to the Extract class,
so d doesn't have to be public.
> extractorcollection.cpp:123
> if (plugin) {
> - Extractor* ex= new Extractor;
> + Extractor* ex= new Extractor(std::move(newExtractor));
> ex->d->m_plugin = plugin;
This whole std::move business is fun, but we could just add an Extractor ctor
that takes an ExtractorPlugin as parameter, no?
(and possibly a QPluginLoader, depending on what we decide about unloading).
Then the loading will be using QPluginLoader here, as before, without a need
for cloning the extractor, which looks strange to me, design wise.
Either move the whole loading code to Extractor, or load here and then
instanciate (as before), but creating two extractor instances (and moving one
to the other) looks weird, overkill, and recipe for trouble.
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D7750
To: mgallien, #frameworks, dfaure
Cc: dfaure, anthonyfieroni
