[Differential] [Requested Changes To] D4020: Latest work on KDev-Clang-Tidy before community polishments

[kfunk (Kevin Funk)]

kfunk requested changes to this revision.
kfunk added a comment.
This revision now requires changes to proceed.


  Just style/minor nitpicks for now, I'll do a more inclusive review later.
  
  Another question: Does this Diff consist of multiple commits or is it a bulk change?

INLINE COMMENTS

> clangtidypreferences.cpp:35
>  {
> -    delete ui;
> +    Ui::ClangTidySettings ui;
> +    ui.setupUi(this);

That's an interesting pattern. You should keep the instance to `Ui::ClangTidySettings` around so you can for instance read properties of one the widgets (e.g. `m_ui->myButton->text())`).

I suggest using a `QScopedPointer<Ui::ClangTidySettings> m_ui` as member.

> parameters.cpp:39
> +    PerProjectSettings projectSettings;
> +    if (m_project != nullptr) {
> +        projectSettings.setSharedConfig(m_project->projectConfiguration());

Style: `m_project != nullptr` -> `m_project`

> parameters.cpp:92
> +
> +    if (m_analiseTempDtors) {
> +        cli << QString("--analyze-temporary-dtors");

Style: I'd remove most of the newlines below here, looks excessive.

Newlines after a block `return`ing from a function is good though.

> parameters.h:73
> +public:
> +    Parameters(KDevelop::IProject* project=nullptr, KDevelop::IDocument* document=nullptr);
> +

Style:  Here and in other files: `A* b = nullptr` instead of `A* b=nullptr`.

You can also install the *uncrustify* code beautifier [1] and then just right-click the file you're editing and select Reformat Source.

http://uncrustify.sourceforge.net/

> parameters.h:75
> +
> +    const QStringList& commandLine()    const {return m_commandLine;}
> +    const QString& filePath()           const {return m_filePath;}

No const-ref on implicitly shared types such as `QStringList`/`QString`/etc.

> parameters.h:80
> +    bool headerFilterMatchesComponent() const {return m_matchHeaderOfTU;}
> +    void setDumpConfig(bool value)            {m_dumpConfig=value; m_commandLine=composeCommandLine();}
> +    QString projectRootDir()            const {return m_projectRootPath.toLocalFile();}

Better group getter/setters together (`setDumpConfig` & `mustDumpToConfigFile`).

Also, make sure they're named similar. Suggestion `setDumpConfigEnabled`, `isDumpConfigEnabled`.

> perprojectconfigpage.cpp:25
> +#include <QMessageBox>
> +#include <algorithm>
>  #include <interfaces/iproject.h>

Style: Group includes in this way:

- File-specific includes
- Newline
- KDE includes
- Newline
- Qt includes
- Newline
- std includes

-> From special to generic includes

Yeah, we don't have this written down anywhere, so there's no clear pattern... :)

> perprojectconfigpage.cpp:70
> +    
> +    if(fromConfig.isEmpty())
> +    {

Style: Here & other places: `if (cond) {`

Again: Better just use uncrustify :)

> perprojectconfigpage.cpp:139
> +    KDevelop::ConfigPage::defaults();
> +    bool restore = m_projectSettings->useDefaults(true);
> +    loadSelectedChecksFromConfig();

`restore` -> `oldUseDefaultsValue`?

> perprojectconfigpage.cpp:143
> +    m_projectSettings->useDefaults(restore);
> +    emit this->changed();
>  }

Style: Here & in other places: `s/this->//`

> perprojectconfigpage.h:75
> +protected:
> +    void joinChecks();
> +    void loadSelectedChecksFromConfig();

This probably needs a bit of api docs. Couldn't guess from the function name what this does.

REPOSITORY
  R218 KDev Clang-Tidy Support

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

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

To: coliveira, antonanikin, tcanabrava, kfunk
Cc: kdevelop-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20170108/a94973e9/attachment-0001.html>