dfaure marked 18 inline comments as done. dfaure added inline comments. INLINE COMMENTS
> kossebau wrote in kmountpoint.cpp:344 > Why change away from auto? Consistency with all other range for loops in kio, and https://wiki.qt.io/Coding_Conventions#auto_Keyword > kossebau wrote in kmountpoint.cpp:429 > `it` -> `mountPointPtr`? Perfect would be renaming the existing `mountpoint` > to `mountPointPath or something, so `mountPoint` could be used as loop var > name. But I also think this would be too invasive WRT commit history and > commit purpose. > Or `mp`, as used in some other code also looping over the same structure, see > below in src/widgets/kpropertiesdialog.cpp picked `mp` > kossebau wrote in kdirsortfilterproxymodel.cpp:119 > Curious: while touching this, would a `static const` or `constexpr` not be > the even better option? > Any rule of thumb known when telling the compiler which way to treat this? > Right now unsure what the compiler even does with a plain const array in a > method without any optimization. It copies the array data onto the stack on > the method invocation, or? Good question. Without optimizations, I assume `static` means generating atomics for thread-safety, while on stack generates a `memcpy` indeed. I checked the assembly on godbolt.org, for that last part. Couldn't see atomics with static because the testcase is all in main(), so no threads involved, but in a library method this is a requirement. With -O2, the generated code is exactly the same. https://godbolt.org/z/Ez8fba So I guess it doesn't matter? > kossebau wrote in kuriikwsfiltereng.cpp:304 > unrelated change, please separate commit Indeed, this whole subdir was in need of code reformatting. Done, https://commits.kde.org/kio/c8c2eff2a2362278acddcc995221176a7788e24e > kossebau wrote in kuriikwsfiltereng.cpp:311 > Unrelated change/fix, please separate commit. Though, is this even correct? > There is no `QString::operator>=(QChar)` & similar, is there? Oh indeed, I thought `c` was a QChar, given its name... I guess this creates a QString from the QChar but yeah I'll revert. > kuriikwsfiltereng.cpp:324 > if (it != ql.end()) > it->clear(); > } the code here uses clear() already for `ql` elements. > kuriikwsfiltereng.cpp:346 > // Generate list of unmatched strings: > QString v = ql.join(QLatin1Char(' ')).simplified(); > and this is the final use of `ql`, null vs empty makes no difference => https://commits.kde.org/kio/ea035cd204dea8cd563155c4256400fe67e8b69c > dropjob.cpp:67 > explicit DropMenu(QWidget *parent = nullptr); > - ~DropMenu(); > + ~DropMenu() override; > Moved all overrides to https://commits.kde.org/kio/54429f8a36aad65088b02eb9d13c3bf78f61f22f > kossebau wrote in kurifilter.cpp:681 > I would make this a separate commit, as this does not exactly match the > commit message and might confuse future history readers. https://commits.kde.org/kio/1660e30f3821204b08b43910738fa52ce1cf0915 REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D24160 To: dfaure, bruns, kossebau Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
