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

[Kevin Funk]

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


  Looks good to me *in general*, but this needs more refactoring IMO (too much logic duplication in Parameters).
  
  @antonanikin Would like your input as well if possible

INLINE COMMENTS

> parameters.h:55
> +
> +    // project settings.
> +    QString m_filePath;

Lots of code/logic duplication here & below between Parameters & PerProjectSettings. => Difficult to maintain.

It looks like all this isn't really necessarily to keep around. We already have PerProjecSettings for this, no? Job only needs m_dumpConfig. Maybe that state could be rather a property on Job itself? Instead of passing Parameters to the Job, pass the executable path + command line yourself? Or just set it from the outside?

> perprojectconfigpage.cpp:46
>  
>      m_availableChecksModel = new QStringListModel();
> +    m_availableChecksModel->setStringList(m_underlineAvailChecks);

Better: `new QStringListModel(this);` -- otherwise leaks

> perprojectsettings.kcfg:16
> +        </entry>
> +        <entry name="AnaliseTempDtors" key="AnaliseTempDtors" type="Bool">
> +            <default>false</default>

Typo: Analise

> job.h:42
>  public:
> -    /**
> -     * \class
> -     * \brief command line parameters.
> -     */
> -    struct Parameters {
> -        QString projectRootDir;
> -        QString executablePath;
> -        QString filePath;
> -        QString buildDir;
> -        QString additionalParameters;
> -        QString analiseTempDtors;
> -        QString enabledChecks;
> -        QString useConfigFile;
> -        QString dumpConfig;
> -        QString enableChecksProfile;
> -        QString exportFixes;
> -        QString extraArgs;
> -        QString extraArgsBefore;
> -        QString autoFix;
> -        QString autoFixError;
> -        QString headerFilter;
> -        QString lineFilter;
> -        QString listChecks;
> -        QString checkSystemHeaders;
> -    };
> -
> -    Job(const Parameters& params, QObject* parent = nullptr);
> +    Job(const Parameters& params, QObject* parent);
>      ~Job() override;

`parent = nullptr` to be consistent with interface from base class.

> replacementparser.h:91
>      ReplacementParser() = default;
> -    explicit ReplacementParser(const QString& yaml_file, const QString& source_file);
> +    explicit ReplacementParser(QString yaml_file, QString source_file);
>      void parse();

Style: Keep const-refs + use camelCase for var names

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, #kdevelop, kfunk
Cc: kdevelop-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20170112/066d4063/attachment.html>