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.
vkrause | |
mlaurent | |
dvratil |
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.
'make test' on my linux system still works
Automatic diff as part of commit; lint not applicable. |
Automatic diff as part of commit; unit tests not applicable. |
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. |
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.
src/vcalformat.cpp | ||
---|---|---|
365 | QLatin1String() suggestion not applied. |