From kde-frameworks-devel Mon Jan 04 08:00:31 2016 From: "David Faure" Date: Mon, 04 Jan 2016 08:00:31 +0000 To: kde-frameworks-devel Subject: Re: Review Request 126620: Fixed most of level 1 and level 2 warnings Message-Id: <20160104080031.6499.3002 () mimi ! kde ! org> X-MARC-Message: https://marc.info/?l=kde-frameworks-devel&m=145189448208510 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--===============4999819612383611835==" --===============4999819612383611835== Content-Type: multipart/alternative; boundary="===============6144104841612157317==" --===============6144104841612157317== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126620/#review90525 ----------------------------------------------------------- Ship it! Yes, C++11 style connect is fine in KF5. However QTimer::singleShot has to keep using the SLOT() syntax for now, to Qt 5.3 compat (until Qt 5.6 is out, then we'll drop Qt 5.3). src/browserextension.cpp (line 257) This won't compile with Qt 5.3, please revert this particular line. - David Faure On Jan. 3, 2016, 9:09 p.m., Artur Puzio wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126620/ > ----------------------------------------------------------- > > (Updated Jan. 3, 2016, 9:09 p.m.) > > > Review request for KDE Frameworks and Aleix Pol Gonzalez. > > > Repository: kparts > > > Description > ------- > > Fixed `warning: QString* being called [-Wclazy-qstring-uneeded-heap-allocations]` by changing to `QStringLiteral`-s. > In `plugin.cpp:96` I haven't fixed the warnings, becouse `QStringLiteral` would give an error. I applied my own optimisation instead. > Fixed `warning: Don't call QList::first() on temporary [-Wclazy-detaching-temporary]` by changeing to `at(0)` > Fixed `warning: Reserve candidate [-Wclazy-reserve-candidates]` by adding a simple `reserve` > Fixed `warning: Old Style Connect [-Wclazy-old-style-connect]` I had to change all of them by hand, becouse of a bug in clazy that I will be reporting. I wasn't able to change 4 Old Style Connects. (3 in `kreadonlypart.cpp`, 1 in `readwritepart.cpp`) > Fixed `warning: KParts::BrowserOpenOrSaveQuestion has dtor but not copy-ctor, copy-assignment [-Wclazy-rule-of-three]`, by adding `Q_DISABLE_COPY`, becouse `BrowserOpenOrSaveQuestionPrivate` isn't copyable > Only warnings that I left are the `[-Wclazy-function-args-by-ref]` warnings, becouse they would involve modifying headers. > > > Diffs > ----- > > src/browserextension.cpp 4b7252c > src/browseropenorsavequestion.h 85dcfa0 > src/browseropenorsavequestion.cpp c1425c1 > src/browserrun.cpp 4756511 > src/browserrun_p.h 4742598 > src/mainwindow.cpp 9b0c3c5 > src/part.cpp 1657561 > src/partmanager.cpp 81bf73f > src/plugin.cpp 97f7d28 > src/scriptableextension.cpp 27676bc > > Diff: https://git.reviewboard.kde.org/r/126620/diff/ > > > Testing > ------- > > Automated tests pass. > Imported note: On my computer I'm runing versions 5.17.0 and it wasn't possible for me to upgrade easly, so I changed the CMakeFiles.txt to compile it. (of course the changed isn't included in this RR) > > > Thanks, > > Artur Puzio > > --===============6144104841612157317== MIME-Version: 1.0 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 7bit
This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126620/

Ship it!

Yes, C++11 style connect is fine in KF5.


However QTimer::singleShot has to keep using the SLOT() syntax for now, to Qt 5.3 compat (until Qt 5.6 is out, then we'll drop Qt 5.3).

src/browserextension.cpp (Diff revision 1) 
void BrowserExtension::slotOpenUrlRequest(const QUrl &url, const KParts::OpenUrlArguments &args, const KParts::BrowserArguments &browserArgs)
257 
    QTimer::singleShot(0, this, SLOT(slotEmitOpenUrlRequestDelayed()));
 257 
    QTimer::singleShot(0, this, &BrowserExtension::slotEmitOpenUrlRequestDelayed);
 
This won't compile with Qt 5.3, please revert this particular line.

- David Faure


On January 3rd, 2016, 9:09 p.m. UTC, Artur Puzio wrote:

Review request for KDE Frameworks and Aleix Pol Gonzalez.
By Artur Puzio.

Updated Jan. 3, 2016, 9:09 p.m.

Repository: kparts

Description 

Fixed warning: QString* being called [-Wclazy-qstring-uneeded-heap-allocations] by changing to QStringLiteral-s.
In plugin.cpp:96 I haven't fixed the warnings, becouse QStringLiteral would give an error. I applied my own optimisation instead.
Fixed warning: Don't call QList::first() on temporary [-Wclazy-detaching-temporary] by changeing to at(0)
Fixed warning: Reserve candidate [-Wclazy-reserve-candidates] by adding a simple reserve
Fixed warning: Old Style Connect [-Wclazy-old-style-connect] I had to change all of them by hand, becouse of a bug in clazy that I will be reporting. I wasn't able to change 4 Old Style Connects. (3 in kreadonlypart.cpp, 1 in readwritepart.cpp)
Fixed warning: KParts::BrowserOpenOrSaveQuestion has dtor but not copy-ctor, copy-assignment [-Wclazy-rule-of-three], by adding Q_DISABLE_COPY, becouse BrowserOpenOrSaveQuestionPrivate isn't copyable
Only warnings that I left are the [-Wclazy-function-args-by-ref] warnings, becouse they would involve modifying headers.

Testing 

Automated tests pass.
Imported note: On my computer I'm runing versions 5.17.0 and it wasn't possible for me to upgrade easly, so I changed the CMakeFiles.txt to compile it. (of course the changed isn't included in this RR)

Diffs

  • src/browserextension.cpp (4b7252c)
  • src/browseropenorsavequestion.h (85dcfa0)
  • src/browseropenorsavequestion.cpp (c1425c1)
  • src/browserrun.cpp (4756511)
  • src/browserrun_p.h (4742598)
  • src/mainwindow.cpp (9b0c3c5)
  • src/part.cpp (1657561)
  • src/partmanager.cpp (81bf73f)
  • src/plugin.cpp (97f7d28)
  • src/scriptableextension.cpp (27676bc)

View Diff
--===============6144104841612157317==-- --===============4999819612383611835== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KS2RlLWZyYW1l d29ya3MtZGV2ZWwgbWFpbGluZyBsaXN0CktkZS1mcmFtZXdvcmtzLWRldmVsQGtkZS5vcmcKaHR0 cHM6Ly9tYWlsLmtkZS5vcmcvbWFpbG1hbi9saXN0aW5mby9rZGUtZnJhbWV3b3Jrcy1kZXZlbAo= --===============4999819612383611835==--