Initial version of Clazy analyzer plugin
ClosedPublic

Authored by antonanikin on Jul 23 2018, 9:07 AM.

Details

Summary

Clazy is a compiler plugin which allows clang to understand Qt semantics. You get more than 50 Qt related compiler warnings, ranging from unneeded memory allocations to misusage of API, including fix-its for automatic refactoring.

https://commits.kde.org/clazy

The plugin allows you to check project's code with clazy checker. Runtime dependencies:

  • clazy-standalone (clazy part)
  • installed clazy docs (used to building checks DB with errors descriptions)
  • make (for parallel checking)
  • compile_commands.json present in project's build directory

Plugin's GUI provides easy way to clazy configuration, enabling/disabling checks and so on.

Test Plan

Works as expected on my neon-useredition system (Ubuntu Xenial) with
Qt 5.11.1, kdevelop (git/master) and clazy (git/master).

Diff Detail

Repository
R32 KDevelop
Branch
kdev_clazy
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1810
Build 1828: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
kossebau added inline comments.Aug 17 2018, 1:08 PM
plugins/clazy/checksdb.cpp
78 ↗(On Diff #38288)

const, please (also good for the range-based foor loop below)

96 ↗(On Diff #38288)

more const :)

plugins/clazy/config/checkswidget.cpp
54 ↗(On Diff #38288)

Curious: why a lambda instead of a member method? What are the adavantages, what the disadvantages?

Possibly a matter of taste and what the code-reading brain is trained to, but I find such long lambdas hard to read right now, and also am not used to this in the code I have seen, so happy to extend my worldview :)

135 ↗(On Diff #38288)

= default

plugins/clazy/config/commandlinewidget.cpp
42 ↗(On Diff #38288)

= default

plugins/clazy/config/projectconfigpage.cpp
127 ↗(On Diff #38288)

= default

plugins/clazy/jobparameters.h
70 ↗(On Diff #38288)

explicit

plugins/clazy/plugin.cpp
67 ↗(On Diff #38288)

IMHO the lambdas could be deduplicated (or perhaps turned into normal class methods, whatever is the better approach).

112 ↗(On Diff #38288)

Other than in if-statements here IMHO an explicit != nullptr makes the code better to read on quick parsing.

227 ↗(On Diff #38288)

be more explicit and ssay who takes care of that: "job automatically deletes itself later", so the code reader does not potentially wrongly expect the plugin code to do it elsewhere.

252 ↗(On Diff #38288)

Build folder as well?

plugins/clazy/plugin.h
39 ↗(On Diff #38288)

Please add here or perhaps in a separate README file in the directory a short description about the approach the plugin takes to make use of clazy, .i.e. the generation of a Makefile which then is used with make and clazy.

plugins/clazy/tests/test_clazyjob.cpp
30 ↗(On Diff #38288)

Please remove module prefix QtTest

82 ↗(On Diff #38288)

A failing test though means something is broken in the code/logic. FAIL sends a wrong message here IMHO.
+1 for QSKIP if runtime dep is not found.

plugins/clazy/utils.cpp
117 ↗(On Diff #38288)

= default

224 ↗(On Diff #38288)

mini optimization: use QLatin1Char for single letter, using the replace(QChar, QLatin1String) overload

239 ↗(On Diff #38288)

split off data member with oen private: section

kossebau edited the summary of this revision. (Show Details)Aug 17 2018, 1:09 PM

Got a crash when running on kdevelop/kdevplatform, seems some normalization of path is needed, have not yet investigated further. Relevant part of backtracce:

#10 0x00007f5f452c97d9 in qt_assert(char const*, char const*, int) (assertion=<optimized out>, file=<optimized out>, line=<optimized out>) at ../../include/QtCore/../../src/corelib/global/qlogging.h:91
#11 0x00007f5f08ca68fd in KDevelop::DocumentRange::DocumentRange(KDevelop::IndexedString const&, KTextEditor::Range const&) (this=0x7fffe8850f90, document=..., range=...) at /home/koder/Kode/kdegit/kf5/extragear/kdevelop/kdevelop/kdevplatform/language/editor/documentrange.h:42
#12 0x00007f5f08ca570e in Clazy::Job::postProcessStderr(QStringList const&) (this=0x5dbc6f0, lines=...) at /home/koder/Kode/kdegit/kf5/extragear/kdevelop/kdevelop/plugins/clazy/job.cpp:174
ASSERT: "document.toUrl() == document.toUrl().adjusted(QUrl::NormalizePathSegments)" in file /home/koder/Kode/kdegit/kf5/extragear/kdevelop/kdevelop/kdevplatform/language/editor/documentrange.h, line 42

Added in job before the call this snippet...

            const auto document = KDevelop::IndexedString(match.captured(1));
qDebug() << "FFFFFAIL" << document.toUrl() << document.toUrl().adjusted(QUrl::NormalizePathSegments);

which got me this before the crash

FFFFFAIL QUrl("file:///home/koder/projects/kdevelop/kdevplatform/language/duchain/navigation/../indexeddeclaration.h") QUrl("file:///home/koder/projects/kdevelop/kdevplatform/language/duchain/indexeddeclaration.h")

Besides that, works as intended for what I got to test :) But will need to do some more, e.g. how fixits work.

Helped myself around that crash by just turning the path into a canonical one, doing this:

const auto filePath = QFileInfo(match.captured(1)).canonicalFilePath();

Not sure if this is the correct fix, more a wild guess to get things going for more testing.

Some more things I noticed:

  • running on the project can result in duplicated problem reports, e.g. if a source file is used as source for multiple build products -> deduplication could be considered as future improvement
  • tooltip on problems in the source code view have strange rendering with too much spacing around the check title
  • tooltips for problems in general have a problem if the help text is very long, resulting in way too big tooltips (at least on old low-dpi screens :) )
  • would be nice to see which checks actually are backed by fix-its, but that needs improvements with clazy I guess to be able to query that? wroth a todo note at least somewhere

Used the plugin already to polish KDevelop codebase some more, cmp. row of commits to 5.3 branch :)

Maintainers: @brauch, @apol, @kfunk, @mwolff What is your take? IMHO would be good to get this in ASAP into 5.3 branch, before the beta..
Works for me (modulo what I mentioned) and the current feature set should be good enough for 5.3 series.

pino added inline comments.Aug 19 2018, 1:31 PM
plugins/clazy/config/commandlinewidget.ui
30 ↗(On Diff #38288)

Since there are still changes to do... this needs to be "Command Line", since it is the title of a group box.
See our guidelines on capitalization: https://hig.kde.org/style/writing/capitalization.html

plugins/clazy/config/globalconfigpage.cpp
39 ↗(On Diff #38288)

This must be i18np(), since there is a variable number.

plugins/clazy/config/globalconfigpage.ui
53 ↗(On Diff #38288)

Please no abbreviations -- see https://hig.kde.org/style/writing/wording.html

60 ↗(On Diff #38288)

Ditto

plugins/clazy/config/projectconfigpage.ui
57 ↗(On Diff #38288)

No contractions please

170–171 ↗(On Diff #38288)
  • no contractions
  • no need to SCREAM
  • "do not blame me": other than being unfriendly, users do not even know who to blame...
188 ↗(On Diff #38288)

"Extra Parameters"

plugins/clazy/job.cpp
98 ↗(On Diff #38288)

Will this work if the sources are in paths with spaces (eg /data/my directory/devel/)?

101 ↗(On Diff #38288)

Ditto, even the clazy command can be somewhere with a path with eg spaces.

plugins/clazy/jobparameters.cpp
156 ↗(On Diff #38288)

"exist"

plugins/clazy/tests/test_clazyjob.cpp
82 ↗(On Diff #38288)

Exactly, it is a runtime dependency: if I build kdevelop and run the test suite with no clazy installed, this test fails. As Friedrich said, a failure means something went wrong, not that an optional tool was not found.

plugins/clazy/utils.cpp
92–94 ↗(On Diff #38288)

FWIW: Okular uses the Discount library for reading markdown documents, showing them as HTML. Also Cantor will do so too: D14738: Add the markdown entry.

Maybe it'd be a good idea to use it -- there are few options:

  • make the library a hard dependency for this plugin (which then would not be built if discount is not found
  • optionally use discount, using this simple fallback otherwise (though with the risk of rotting)
  • optionally use discount, and show no documentation if not found

@antonanikin So given the 5.3 release preparation came over us a bit sudden, we are under time pressure here a little. There is still a small time window to get this in, as just agreed on on irc with @brauch last chance is Thursday Aug 23rd evening CEST. String freeze will be delayed extra for this plugin until then :)

Do you think you will manage to get the current review feedback handled until then, ideally with another round of review?

Otherwise the plugin would have to wait for 5.4 and only be merged to master. In that worst case though I would propose to do some independent plugin releases during the next weeks/months, similar to what I started with kdev-clang-.tidy, so users can already enjoy the featzre. Would happily work together with you.

But for now let's focus on still getting this plugin in for 5.3 :)

antonanikin marked 28 inline comments as done.Aug 20 2018, 5:27 AM

FWIW: Okular uses the Discount library for reading markdown documents, showing them as HTML. Also Cantor will do so too: D14738: Add the markdown entry.

Maybe it'd be a good idea to use it -- there are few options:

  • make the library a hard dependency for this plugin (which then would not be built if discount is not found
  • optionally use discount, using this simple fallback otherwise (though with the risk of rotting)
  • optionally use discount, and show no documentation if not found

I think about this library before writing current custom parser. It looks good and easy to use but adds extra dependency, especially for AppImage/Windows installer. Since my skills for such installers is clear to zero I decide to skip it at current time. Also if we want to add Clazy plugin into 5.3 release we have some time limits. So I suggest to use current approach before 5.3 release 'as is' and then make another patch for Discount usage instead manual parser.

plugins/clazy/config/checkswidget.cpp
135 ↗(On Diff #38288)

Ui::ChecksWidget is incomplete type for default destructor.

plugins/clazy/config/commandlinewidget.cpp
42 ↗(On Diff #38288)

Ui::CommandLineWidget is incomplete type for default destructor.

plugins/clazy/config/projectconfigpage.cpp
127 ↗(On Diff #38288)

JobParameters is incomplete type for default destructor.

plugins/clazy/job.cpp
98 ↗(On Diff #38288)

Oh... Fixed, thanks.

plugins/clazy/plugin.cpp
252 ↗(On Diff #38288)

We may want to check generated files, for example. Or you think we should disable such checking?

plugins/clazy/tests/test_clazyjob.cpp
82 ↗(On Diff #38288)

Sorry for not the exact formulation - clazy-standalone is runtime dependency for the plugin (real code analysis). This test will works without this executable - here we test only output and error parsing without executable calls. So should we skip the test when clazy-standalone is missing?

antonanikin marked 6 inline comments as done.Aug 20 2018, 5:36 AM

@kossebau, @pino, thanks a lot for detailed review. Will be great to add new clazy plugin into upcoming 5.3 release.

pino added a comment.Aug 20 2018, 5:44 AM

FWIW: Okular uses the Discount library for reading markdown documents, showing them as HTML. Also Cantor will do so too: D14738: Add the markdown entry.

Maybe it'd be a good idea to use it -- there are few options:

  • make the library a hard dependency for this plugin (which then would not be built if discount is not found
  • optionally use discount, using this simple fallback otherwise (though with the risk of rotting)
  • optionally use discount, and show no documentation if not found

I think about this library before writing current custom parser. It looks good and easy to use but adds extra dependency, especially for AppImage/Windows installer.

Note that, unless you pack clazy as well, the plugin will be unusable for AppImage/Windows.

plugins/clazy/tests/test_clazyjob.cpp
82 ↗(On Diff #38288)

Clazy::Job creates a makefile that itself invokes clazy-standalone. Also, the outputs that are checked for (e.g. stderrOutput1 & friends) are printed by clazy-standalone, aren't they?

  • Test fixes
  • More const
  • Codestyle fixes
  • Escape path spaces for correct make run
  • i18n fixes
  • Fix crash during error parsing
  • Fix clazy "empty-qstringliteral" warning
  • Add readme

Note that, unless you pack clazy as well, the plugin will be unusable for AppImage/Windows.

Hmm... I thought that clazy/clang/gcc/etc should be installed by user and we will only use system tools from our plugin(s).

plugins/clazy/tests/test_clazyjob.cpp
82 ↗(On Diff #38288)

Clazy::Job creates a makefile only when it created with "right" constructor. JobTester uses "fake" empty one and can't be run. In our test we call only postProcessStdout() and postProcessStderr() methods - so this test is only "parser test". Such approach also used by Cppcheck plugin - we tests only parser without execution of any external tools/libs.

stderrOutput1 & friends are taken form clazy-standalone output for some real code and then adapted for our tests (filename, line and column numbers are changed).

Sadly too tired right now to grasp the whole codebase for a decided last commnet. And not an expert in KDevelop API for analysis tools :)
But from what I tested today with latest codebase, and what I understood from the code, this should be good to be merged (to 5.3 branch).

So my +1 for merging on Wednesday, if no-one else has further objections (will see to review tomorrow morning again with fresh mind :) ).

plugins/clazy/checksdb.cpp
80 ↗(On Diff #38288)

ChecksDB constructor is not too often called,so no need to cache this in a static. instead move just out of the loop.

98 ↗(On Diff #38288)

same, move out of loop instead

plugins/clazy/config/checkswidget.cpp
135 ↗(On Diff #38288)

Only if you use that in the class declaration, right? Actually meant this could be done here, like this:

ChecksWidget::~ChecksWidget() = default;
149 ↗(On Diff #38288)

& missing for const reference type

207 ↗(On Diff #38288)

If possible, prepare already for no-implicit ascii->QString/QChar conversion, using QLatin1Char here and respective elsewhere.

plugins/clazy/config/checkswidget.h
57 ↗(On Diff #38288)

typo che_c_ks

plugins/clazy/config/checkswidget.ui
74 ↗(On Diff #40026)

Finish sentence with a "."

plugins/clazy/job.cpp
45 ↗(On Diff #38288)

This comment might be better as api dox on the method in the class declaration.

98 ↗(On Diff #38288)

Hm, possibly instead of putting all files into a single line, adding them line by line might be better, with many files and deep project paths?

Also might be easier for the code reader if the escaping is done here, it comes a little by surprise that sources() already delivered the escaped variants.

plugins/clazy/jobparameters.cpp
113 ↗(On Diff #38288)

Why not using QStandardPaths::locate()?

plugins/clazy/jobparameters.h
31 ↗(On Diff #38288)

empty lines before/after could be removed, following pattern elsewher:

namespace KDevelop
{
classIProject;
}
plugins/clazy/plugin.cpp
63 ↗(On Diff #38288)

Ideally the loading of the db could be delayed until needed, given this is in the hotpath of starting.

252 ↗(On Diff #38288)

Hm, unsure. Given we cannot influence the generating tools, such checks might not help that much. But perhaps some people still prefer to be able to do that, so perhaps we can keep it, does not hurt anyone I guess :)

plugins/clazy/problemmodel.cpp
82 ↗(On Diff #38288)

auto& o, no need to create another non-trivial pointer object copy.

plugins/clazy/utils.cpp
71 ↗(On Diff #38288)

static is not really needed here, there is not anything special about this over the other QStringLiterals?

antonanikin marked 14 inline comments as done.Aug 21 2018, 5:33 AM
antonanikin added a subscriber: arrowd.

@arrowd, can you test the plugin on your FreeBSD system? My main question - is BSD make works well with our generated makefile.

plugins/clazy/config/checkswidget.cpp
135 ↗(On Diff #38288)

Heh, I didn't know about such variant. Thanks :)

plugins/clazy/plugin.cpp
63 ↗(On Diff #38288)

Done. Now we check and load db at clazy start and at project config open.
Not so beautiful but works.

antonanikin marked 2 inline comments as done.
  • Inline comments and i18n fixes
  • Problems deduplication
  • Add indication about Clazy run in background (related to bug 375557)
  • Add indication about Clazy run in background (related to bug 375557)

This commit based on D4816 and seems to be useful - now we show message at analysis start and also at end when no problems was detected.

Old version is very "silent" - it's often impossible to understand when analysis is finished when no errors was detected - user must manually open "Testing" output view and check messages about analysis completion.

  • Small codestyle fix
kossebau accepted this revision.EditedAug 21 2018, 4:55 PM

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

plugins/clazy/config/projectconfigpage.ui
57 ↗(On Diff #38288)

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

67 ↗(On Diff #38288)

"Disable checks not compatible with Qt 4.,"

plugins/clazy/job.cpp
264 ↗(On Diff #38288)

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

plugins/clazy/problemmodel.cpp
83 ↗(On Diff #38288)

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 )

106 ↗(On Diff #38288)

same auto& problem

This revision is now accepted and ready to land.Aug 21 2018, 4:55 PM
antonanikin marked 5 inline comments as done.Aug 22 2018, 2:50 AM
  • Inline comments fixes
This revision was automatically updated to reflect the committed changes.

Thank you all for the detailed code review. I just merge this patch to 5.3 branch.

If you have the opportunity then please check the correctness and style of UI-strings - my english skills is unfortunately not so good :(.