[Differential] [Updated] D2840: First diff for kdev-clang-tidy

[kfunk (Kevin Funk)]

kfunk added a comment.


  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.

INLINE COMMENTS

> ClangFormatAll.cmake:13
> +if(NOT CLANG_FMT_CMD)
> +    message(ERROR "clang-format not found!")
> +else()

Does that need to be an error?

> perprojectconfigpage.h:64
> +    QIcon icon() const override;
> +    void setList(QStringList list);
> +    void setActiveChecksReceptorList(QStringList* list);

const&

> perprojectconfigpage.h:65
> +    void setList(QStringList list);
> +    void setActiveChecksReceptorList(QStringList* list);
> +

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?

> job.cpp:151
> +    switch (e) {
> +    case QProcess::FailedToStart:
> +        message = i18n("Failed to start Clangtidy from %1.", commandLine()[0]);

Handling those QProcess errors should probably be moved to the KDevelop::OutputExecuteJob class in kdevplatform?

> test_clangtidyparser.cpp:53
> +    QTextStream ios(&output_example_file);
> +    QStringList clangtidy_example_output;
> +    QString line;

Style: Use camel-case style var names here

REPOSITORY
  R218 KDev Clang-Tidy Support

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: coliveira, mwolff, antonanikin, apol, kfunk
Cc: kdevelop-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20160930/20f44c09/attachment.html>