First diff for kdev-clang-tidy
ClosedPublic
Actions

Authored by coliveira on Sep 22 2016, 11:56 PM.

Details

Reviewers

apol
antonanikin
kfunk

Summary

Hi guys, let's get it started! think you guys know better than me who should be looking into this. Any suggestions are welcome.

Diff Detail

Repository

R218 KDev Clang-Tidy Support

Lint

Lint Skipped

Unit

Unit Tests Skipped

coliveira updated this revision to Diff 6885.Sep 22 2016, 11:56 PM

coliveira retitled this revision from to First diff for kdev-clang-tidy.

coliveira updated this object.

coliveira edited the test plan for this revision. (Show Details)

coliveira added reviewers: mwolff, kfunk, antonanikin, apol.

coliveira set the repository for this revision to R218 KDev Clang-Tidy Support.

Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptSep 22 2016, 11:56 PM

This looks really nice for a start.

I've just had a very brief first look.

General remarks:

Use const& where useful (e.g. for setFoo(QString foo -> setFoo(const QString& foo))
I'd camel case the term 'clangtidy' everywhere in source code. That means Clangtidy -> ClangTidy`, or clangtidy -> clangTidy, etc.

cmake/ClangFormatAll.cmake
13	Does that need to be an error?
src/config/perprojectconfigpage.h
64	const&
65	I'm not too fond of this API (`QStringList*`). Maybe it makes more sense to use a property for the receptor list and communicate changes via signal/slots?
src/plugin/job.cpp
151	Handling those QProcess errors should probably be moved to the KDevelop::OutputExecuteJob class in kdevplatform?
tests/test_clangtidyparser.cpp
53	Style: Use camel-case style var names here

Looks good, thanks.

I also had a very brief first look, and my remarks are:

Your TODO about removing hard-coded CLANG_TIDY_PATH - good idea, do it.

Check code reworked from kdev-cppcheck for unused parts. My main remark - confusing comments, which seems to be an incorrect for your app (see job.cpp for example).

Per-project configuration code looks a bit complicated and redundant. I think it will be good to reimplement per-project configuration with using standard KConfigSceleton approach. This way allows you to drop(or simplify) some "boring" code like a manually created apply() methods for Config Pages. My last version of kdev-cppcheck (differential D2844) already uses this approach.

src/plugin/plugin.cpp
251	it's better to use new API: `core()->languageController()->problemModelSet()->showModel(problemModelName);`

Thanks for the comments, guys. I already made most of the suggestions, I think.
About the KConfigSkeleton approach I need to study more to understand how it works. In the meantime, I left the PerProjectConfigPage almost in the same way it was, except for the pointer API, which Kevin Funk suggested to change.
As soon as I understand the KConfigSkeleton enough to reimplement the PerProjectConfigPage I'll submit another patch.

Thank you, guys!

cmake/ClangFormatAll.cmake
13	Well, WARNING should be enough, since it's just for developers to auto format code.
src/config/perprojectconfigpage.h
65	Now PerProjectConfigPage is the owner of the list of selected checks and it passes this list by value by emitting a signal which is connected to a slot on the Plugin class. Do you think it is enough?
src/plugin/job.cpp
151	I saw KDevelop::OutputExecuteJob code and realized that I was doing nothing special, so I preferred to drop this method.
src/plugin/plugin.cpp
251	Cleaner API. I like it.

Improved style by preferring camel case identifiers, cleaned wrong comments, changed ClangFormat target to issue a WARNING instead of ERROR in case of clang-format executable not being found, and changed the API on PerProjectConfigPage to issue a signal when the selection of checks change instead of using a QStringList pointer owned by someone else.

I didn't get a clue on how to use KConfigSkeleton in PerProjectConfigPage because the list of checks is generated during runtime, by running clang-tidy and parsing its output. I had a hard time trying to use this and still have the list of checks not hard-coded inside the plugin code, which I believe it's the right way to do the plugin, since the list of checks may change from one version to another of clang-tidy. Ideas are welcome.

Hi guys!

I think I got the idea on using KConfigSkeleton, as suggested by Anton Anikin.
By the way, I made some improvements on the UI and added a few features to support the changes in the UI.
I have quite a long diff to submit for review.

Would it be appropriate to add the diff in this revision or should we close this review and create another one with this new diff?

In D2840#56698, @coliveira wrote:

Hi guys!

I think I got the idea on using KConfigSkeleton, as suggested by Anton Anikin.
By the way, I made some improvements on the UI and added a few features to support the changes in the UI.
I have quite a long diff to submit for review.

Would it be appropriate to add the diff in this revision or should we close this review and create another one with this new diff?

Please create a new review.

Cheers,
Kevin

kfunk accepted this revision.Oct 15 2016, 7:27 PM

kfunk edited edge metadata.

This revision is now accepted and ready to land.Oct 15 2016, 7:27 PM

kfunk closed this revision.Oct 15 2016, 7:27 PM

Revision Contents
Changeset List

		Path
M		.clang-tidy (94 lines)
M		.gitignore (3 lines)
M		CMakeLists.txt (4 lines)
M		cmake/ClangFormatAll.cmake (2 lines)
M		src/config/clangtidypreferences.h (10 lines)
M		src/config/clangtidypreferences.cpp (24 lines)
M		src/config/configgroup.h (7 lines)
A	M	src/config/configgroup.h.in (101 lines)
M		src/config/configgroup.cpp (2 lines)
M		src/config/perprojectconfigpage.h (12 lines)
M		src/config/perprojectconfigpage.cpp (60 lines)
M		src/config/ui/clangtidysettings.ui (10 lines)
M		src/config/ui/perprojectconfig.ui (4 lines)
M		src/parsers/clangtidyparser.h (6 lines)
M		src/parsers/clangtidyparser.cpp (38 lines)
M		src/parsers/replacementparser.h (24 lines)
M		src/parsers/replacementparser.cpp (56 lines)
M		src/plugin/job.h (4 lines)
M		src/plugin/job.cpp (74 lines)
M		src/plugin/plugin.h (13 lines)
D	M	src/plugin/plugin.h.in (107 lines)
M		src/plugin/plugin.cpp (82 lines)
M		src/qCDebug/debug.cpp (2 lines)
M		tests/CMakeLists.txt (13 lines)
M		tests/data/nihon_plugin.cpp (56 lines)
M		tests/data/plugin.cpp (54 lines)
M		tests/test_clangtidyjob.h (2 lines)
M		tests/test_clangtidyjob.cpp (14 lines)
M		tests/test_clangtidyparser.h (2 lines)
M		tests/test_clangtidyparser.cpp (24 lines)
M		tests/test_plugin.h (2 lines)
M		tests/test_plugin.cpp (10 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	6885		Sep 22 2016, 11:51 PM	★	★
Diff 2	7239		Oct 10 2016, 1:02 AM	★	★

First diff for kdev-clang-tidyClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 7239

.clang-tidy

.gitignore

CMakeLists.txt

cmake/ClangFormatAll.cmake

src/config/clangtidypreferences.h

src/config/clangtidypreferences.cpp

src/config/configgroup.h

src/config/configgroup.h.in

src/config/configgroup.cpp

src/config/perprojectconfigpage.h

src/config/perprojectconfigpage.cpp

src/config/ui/clangtidysettings.ui

src/config/ui/perprojectconfig.ui

src/parsers/clangtidyparser.h

src/parsers/clangtidyparser.cpp

src/parsers/replacementparser.h

src/parsers/replacementparser.cpp

src/plugin/job.h

src/plugin/job.cpp

src/plugin/plugin.h

src/plugin/plugin.h.in

src/plugin/plugin.cpp

src/qCDebug/debug.cpp

tests/CMakeLists.txt

tests/data/nihon_plugin.cpp

tests/data/plugin.cpp

tests/test_clangtidyjob.h

tests/test_clangtidyjob.cpp

tests/test_clangtidyparser.h

tests/test_clangtidyparser.cpp

tests/test_plugin.h

tests/test_plugin.cpp

First diff for kdev-clang-tidy
ClosedPublic
Actions

Revision Contents
Changeset List