D7750: fix crash when more than one instances of ExtractorCollection are destructed

[David Faure]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170924/0a1c5f8c/attachment-0001.html>