Investigate warnings and fix where it's possible and needed (kdevplatform) [1]
ClosedPublic

Authored by spencerb on Dec 3 2016, 3:39 PM.

Diff Detail

Repository
R33 KDevPlatform
Lint
Lint Skipped
Unit
Unit Tests Skipped
spencerb updated this revision to Diff 8723.Dec 3 2016, 3:39 PM
spencerb retitled this revision from to Investigate warnings and fix where it's possible and needed (kdevplatform) [1].
spencerb updated this object.
spencerb edited the test plan for this revision. (Show Details)
spencerb added reviewers: KDevelop, kfunk, ematirov.
spencerb set the repository for this revision to R33 KDevPlatform.
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptDec 3 2016, 3:39 PM
ematirov added inline comments.Dec 3 2016, 4:06 PM
language/classmodel/documentclassesfolder.cpp
233

Testing for CodeModelItem::Unknown makes no sense there since it's constant (0).
The problem of original check is that (item.kind & 0) == 0 with any value of item.kind, so it doesn't check anything.

Probably solutions are:

  1. check if item.kind is equal to CodeModelItem::Unknown instead of using bitwise operation there;
  2. change value of CodeModelItem::Unknown to some power of 2 which is not used yet in CodeModelItem::Kind values.

Probably first one will go but I'll prefer to hear @kfunk's opinion on that

spencerb updated this revision to Diff 8724.Dec 3 2016, 4:34 PM
spencerb updated this revision to Diff 8725.Dec 3 2016, 4:42 PM
ematirov edited edge metadata.Dec 3 2016, 4:53 PM

Thanks! Just some more nitpicks.

plugins/cvs/cvsproxy.cpp
188

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

plugins/quickopen/expandingtree/expandingdelegate.cpp
134

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.

plugins/testview/testview.cpp
243

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

spencerb updated this revision to Diff 8727.Dec 3 2016, 5:05 PM
spencerb edited edge metadata.
ematirov added inline comments.Dec 3 2016, 5:12 PM
plugins/quickopen/expandingtree/expandingdelegate.cpp
134

And that's one, please. It's really better to use enum by name and not by value.

plugins/testview/testview.cpp
243

It should be !itemsForProject.isEmpty(), not just itemsForProject.isEmpty(). Now it works like:
Check if list is empty (does not contain items) and if it's empty return first item; which is incorrect.

spencerb updated this revision to Diff 8731.Dec 3 2016, 5:18 PM

Got it :)

spencerb updated this revision to Diff 8734.Dec 3 2016, 5:43 PM
spencerb updated this revision to Diff 8735.Dec 3 2016, 5:46 PM
spencerb updated this revision to Diff 8736.
spencerb removed R33 KDevPlatform as the repository for this revision.
spencerb set the repository for this revision to R33 KDevPlatform.
spencerb updated this revision to Diff 8737.Dec 3 2016, 5:51 PM

LGTM!
Thanks, good work.

kfunk accepted this revision.Dec 5 2016, 7:52 AM
kfunk edited edge metadata.

Thanks!

Will push.

This revision is now accepted and ready to land.Dec 5 2016, 7:52 AM
This revision was automatically updated to reflect the committed changes.