dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> discovery.cpp:31
> +Discoverer::Discoverer() = default;
> +Discoverer::~Discoverer() = default;
Lines 30-31 duplicate lines 23-28, surely this can't link without errors?
> dnssddiscoverer.cpp:29
> +
> +KIO::UDSEntry DNSSDDiscovery::toEntry()
> +{
All toEntry() methods should be const, no?
> dnssddiscoverer.cpp:48
> + QUrl u;
> + u.setScheme(QStringLiteral("smb://"));
> + u.setHost(m_service->hostName());
smb, not smb://
> dnssddiscoverer.cpp:74
> + // RemoteService::Ptr is useless here.
> + for (const auto &it : qAsConst(m_services)) {
> + if (*service == *it) {
"it" is a confusing name for the reader, but it's not an iterator. It's a
service pointer.
Maybe call it servicePtr?
Then *servicePtr will make it clear that you're comparing services instances.
> wsdiscoverer.cpp:287
> + QUrl u;
> + u.setScheme(QStringLiteral("smb://"));
> + u.setHost(m_remote);
"smb"
> dfaure wrote in wsdiscoverer.cpp:128
> Needs a const local var to avoid a detach. Yes, range-for is annoying for Qt
> containers.
When the local variable is const, you don't need qAsConst on top :-)
REPOSITORY
R320 KIO Extras
REVISION DETAIL
https://phabricator.kde.org/D25682
To: sitter, dfaure, #frameworks, #dolphin
Cc: bcooksley, ngraham, caspermeijn, davidedmundson, kde-frameworks-devel,
kfm-devel, pberestov, iasensio, fprice, LeGast00n, MrPepe, fbampaloukas,
alexde, GB_2, Codezela, feverfew, meven, michaelh, spoorun, navarromorales,
firef, andrebarros, bruns, emmanuelp, mikesomov
