D14288: Initial version of Clazy analyzer plugin

[Friedrich W. H. Kossebau]

kossebau accepted this revision.
kossebau added a comment.
This revision is now accepted and ready to land.


  Another scan over the code, no show-stoppers seen (though I might be blind by now).  I am a litle unsure about all the UI strings, they might see some more brush over. Though then the same is true for strings in the current KDevelop codebase, so not such a bugger.
  From the functionality POV, I would have some proposals for changes, but as shown by my usage of the plugin for KDevelop's own codebase, the plugin can be used successfully for its purpose as it is. And with feedback from users of 5.3 then things could be developed further.
  
  So when it comes to me, I would be fine by merging the patch as is (ideally with last nitpicks solved). Any further clean-up if needed could then be done inside the 5.3 branch.
  
  I propose to still wait with the string freeze until Thursday, so people building from master/5.3 have two days of chance to give feedback on the UI. Given @brauch passed settings the string freeze day here on to me, let's have it like that :)
  
  @antonanikin So if @kfunk @mwolff or @apol (as maintainers around currently) do not have any further comments/objects in the next few hours, please merge this to the 5.3 branch tonight :)
  @pino  Also would be glad for some last UI string review by you if needed.

INLINE COMMENTS

> projectconfigpage.ui:56
> +            <property name="toolTip">
> +             <string>Won't emit warnings for non-Qt files, or in other words, if -DQT_CORE_LIB is missing</string>
> +            </property>

All tooltip texts should end with a full-stop. (hm, cannot find the respective mention in any hig guide, just remember it is like that). 
Also "Won't" -> "Do not".

For some general guidelines see https://hig.kde.org/style/writing/wording.html

> projectconfigpage.ui:66
> +            <property name="toolTip">
> +             <string>Turns off checks not compatible with Qt 4</string>
> +            </property>

"Disable checks not compatible with Qt 4.,"

> job.cpp:263
> +    setPercent(100);
> +    postProcessStdout({QString("Elapsed time: %1 s.").arg(m_timer->elapsed()/1000.0)});
> +

QString(...) _> QStringLiteral(...)

> problemmodel.cpp:82
> +{
> +    for (auto problem : qAsConst(m_problems)) {
> +        if (newProblem->source() == problem->source() &&

What I mean what I prefer is this:

  for (auto& problem : qAsConst(m_problems)) { 

Reason is that `KDevelop::IProblem::Ptr` is not just a plain raw pointer, but a slightly more complex datatype with a non-trivial copy constructor, which would be used in case of just `auto problem`).
By using a reference to the Ptr object we spare the invocations of that copy constructor call per loop step (cmp. `QExplicitlySharedDataPointer(const QExplicitlySharedDataPointer<X> &o)` code), which saves again some cpy cycles. Not a big gain here indeed, but keeping this as pattern ensures that code in other places which are more a hotpath default to the less expensive variant.
(BTW, clazy warns about that in the range-loop check :P )

> problemmodel.cpp:105
> +
> +    for (auto problem : problems) {
> +        if (problemExists(problem)) {

same auto& problem

REPOSITORY
  R32 KDevelop

BRANCH
  kdev_clazy

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

To: antonanikin, #kdevelop, kossebau
Cc: arrowd, mwolff, apol, kfunk, brauch, pino, kossebau, kdevelop-devel, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20180821/17e8a75c/attachment-0001.html>