BUG: 413195
Details
Diff Detail
- Repository
- R875 Falkon
- Branch
- cmd-urls
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 23352 Build 23370: arc lint + arc unit
In case you're wondering, Yes, I'm interested in becoming a GSoC student. I've contributed to different KDE projects including this year's SoK. However, this is my first patch to Falkon. Once this is done, you can recommend another bug to fix if you prefer.
Also, full name is 'Abdel-Rahman Abdel-Rahman'. I'll try to contact the sysadmin to change my name on Phabricator since I can't change it myself. :/
Thanks!
src/lib/app/browserwindow.cpp | ||
---|---|---|
255 | Just use takeLast() from m_startUrls instead of using iterator. | |
322 | coding style | |
src/lib/app/browserwindow.h | ||
80 | No need for move semantics here. | |
src/lib/app/commandlineoptions.cpp | ||
201–214 | const QStringList args = parser.positionalArguments(); for (QString url : args) { ... } | |
src/lib/app/mainapplication.cpp | ||
442–447 | const QUrl &startUrl | |
446 | two empty lines |
src/lib/app/browserwindow.cpp | ||
---|---|---|
255 | We can't use use takeLast on a const QList. cend() is meant to be used as a default when no URLs are given. |
src/lib/app/browserwindow.cpp | ||
---|---|---|
255 | That's why you should declare it non-const and clear the list in postLaunch as you process the urls. m_startUrls is no longer used after startup. |
src/lib/app/browserwindow.cpp | ||
---|---|---|
322 | That contradicts what @drosca said: |
src/lib/app/browserwindow.cpp | ||
---|---|---|
322 | A single takeFirst()/takeLast() is ok, but a loop over takeFirst() or takeLast() is *really* suboptimal. My suggestion was a single take, and then a C++11 range-for. |
src/lib/app/commandlineoptions.cpp | ||
---|---|---|
202 | I hadn't realized that this code actually modified url (hence the const in my suggestion). Modifying the args list is a bit of an unexpected side effect. I think for (QString url : args) would indeed be better then, or the more traditional for (const QString &arg : args) { QString url = arg; ..... } so that it doesn't look like someone simply forgot a const &... | |
src/lib/app/mainapplication.cpp | ||
453 | Note that copying QList is just increasing/decreasing a refcount, this isn't a full copy like what would happen with std::list or std::vector. The traditional Qt way is to pass a const & in those methods, and copy into a member variable at the end of the chain where you end up in a constructor. Cheap copy. The possibly more modern way is indeed pass by value and std::move, but only for constructors; don't do this for setters. This code is maintained by drosca though, so if he doesn't like std::move, use const & everywhere. |