KCalCore Clazy fixes
ClosedPublic

Authored by winterz on Apr 28 2019, 5:26 PM.

Details

Summary

Not all issued reported in 'level1' checks are here, but this is most of them.

notice I also re-wrote the recurrence code in vcalformat... clazy led me there and I didn't like the what I saw so I improved the code.

Test Plan

'make test' on my linux system still works

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
winterz created this revision.Apr 28 2019, 5:26 PM
Restricted Application added a project: KDE PIM. · View Herald TranscriptApr 28 2019, 5:26 PM
Restricted Application added a subscriber: kde-pim. · View Herald Transcript
winterz requested review of this revision.Apr 28 2019, 5:26 PM
dvratil requested changes to this revision.Apr 28 2019, 8:50 PM
dvratil added a subscriber: dvratil.
dvratil added inline comments.
src/occurrenceiterator.cpp
126

Since you are touching this, should it be const auto & for clarity? Also couple more times below in this patch.

src/recurrencerule.cpp
2078

Should this be in an else branch? Otherwise bydays will be appended twice if the condition above is true

src/vcalformat.cpp
363

You could use leftRef() and compare with a QLatin1String() to reduce allocations, since you are touching this code.

788

This looks identical to the code above, should it be moved into its own function? (not necessarily in this review, but if you feel bored ;-))

807

Same as above.

This revision now requires changes to proceed.Apr 28 2019, 8:50 PM

I will fix those 'auto' variables. I had planned a clang-tidy round of cleanups, but I can do the auto stuff here.
your other suggestions are also good. I'll handle them.

winterz marked 5 inline comments as done.Apr 29 2019, 10:01 PM
winterz added inline comments.
src/recurrencerule.cpp
2078

I'm not seeing what you're seeing. I'll revert the logic back

src/vcalformat.cpp
788

too much work at this time. huge amounts of code in the file really needs refactoring. I'll skip it.

winterz updated this revision to Diff 57221.Apr 29 2019, 10:12 PM
winterz marked 2 inline comments as done.

Dan's comments and suggestions

ach added a subscriber: ach.Apr 30 2019, 2:18 PM
ach added inline comments.
src/vcalformat.cpp
365

QLatin1String() suggestion not applied.

winterz updated this revision to Diff 57287.Apr 30 2019, 7:28 PM

missing changes for QLatin1String()
thanks for noticing Achim

dvratil accepted this revision.May 4 2019, 7:45 AM

Sorry for the delay. Thanks for the improvements!

This revision is now accepted and ready to land.May 4 2019, 7:45 AM
This revision was automatically updated to reflect the committed changes.