D27655: Open all URLs in command line arguments

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> dfaure wrote in commandlineoptions.cpp:202
> const QString &url

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 &...

> abogical wrote in mainapplication.cpp:453
> This will invoke a copy of the list, which is what I'm trying to avoid.

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.

REPOSITORY
  R875 Falkon

REVISION DETAIL
  https://phabricator.kde.org/D27655

To: abogical, #falkon, drosca
Cc: dfaure, drosca, SGOrava, #falkon, falkon, allknow, grune, akilgus, siraj_qazi, saishm, anmolgautam, iodelay, spoorun, ptabis, navarromorales, clivej, mparillo
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/falkon/attachments/20200328/7b645d31/attachment.html>