[Differential] [Updated] D3580: Investigate warnings and fix where it's possible and needed (kdevplatform) [1]

[ematirov (Mikhail Ivchenko)]

ematirov added a comment.


  Thanks! Just some more nitpicks.

INLINE COMMENTS

> cvsproxy.cpp:188
>      }
> -    if (job) delete job;
> +    delete job;
>      return nullptr;

There are more such cases in cvsproxy.cpp. (Them are listed in file with warnings). Could you fix them too please?

> expandingdelegate.cpp:134
>      s.setHeight( widgetSize.height() + s.height() + 10 ); //10 is the sum that must match exactly the offsets used in ExpandingWidgetModel::placeExpandingWidgets
> -  } else if( model()->isPartiallyExpanded( index ) ) {
> +  } else if( model()->isPartiallyExpanded( index ) != 0) {
>      s.setHeight( s.height() + 30 + 10 );

Please use "NotExpandable" there (and in other cases) instead of constant there since it's enum. The main idea of problem there is that order of values in enum can be changed, so NotExpandable will become no-zero and all these ifs will not work as expected.

> testview.cpp:243
> +    QList<QStandardItem*> stditemlist = m_model->findItems(project->name());
> +    if (stditemlist.size() > 0) {
> +        return stditemlist.at(0);

Let's use just !stditemlist.IsEmpty() and stditemlist.first().
And probably "itemsForProject" or something like so will be more meaningful name for that. ;)

REPOSITORY
  R33 KDevPlatform

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

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

To: spencerb, #kdevelop, kfunk, ematirov
Cc: kdevelop-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20161203/be94dc68/attachment-0001.html>