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