GIT_SILENT
Details
Test pass, KXMLGUI-based apps showed no regressions incl. editing of
toolbars & shortcuts
Diff Detail
- Repository
- R263 KXmlGui
- Branch
- morerangebasedforloops
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 17123 Build 17141: arc lint + arc unit
src/kedittoolbar.cpp | ||
---|---|---|
1566 ↗ | (On Diff #66949) | No qAsConst? If done by intention, maybe keep the iterator version? |
src/kgesture.cpp | ||
117 ↗ | (On Diff #66949) | qAsConst? |
131 ↗ | (On Diff #66949) |
|
165 ↗ | (On Diff #66949) | QPoint & |
src/kmainwindow.cpp | ||
349 ↗ | (On Diff #66949) | qAsConst? |
src/kshortcutseditor.cpp | ||
715 ↗ | (On Diff #66949) | Maybe make it a QVector. Or simply an initializer list. |
src/kswitchlanguagedialog_p.cpp | ||
373 ↗ | (On Diff #66949) | qAsConst |
src/ktoolbar.cpp | ||
187 ↗ | (On Diff #66949) | Let's say values detaches here, then the changes value doesn't have any effect on the original one. |
376 ↗ | (On Diff #66949) | constexpr? |
Thanks for review work, @dhaumann :) Seems we have some differences though, so let's try to align knowledge:
When it comes to by-value or by reference of the loop range element value, I learned that similar rules as for arguments are valid: small size in bytes & trivial copy constructor -> value, otherwise reference. QPoint thus would be candidate for by-value. Would I need to improve my knowledge?
Also I have learned that using for(T& t : tContainer) is fine for iterating through a container to change its values, and preferred over its short expression as well. Used to in other places, so surprised by the comments.
src/kedittoolbar.cpp | ||
---|---|---|
1566 ↗ | (On Diff #66949) | By intention, as the elements of the vector are modified. |
src/kgesture.cpp | ||
117 ↗ | (On Diff #66949) | Again, members of d->m_shape are changed (thus also the QPoint&) |
131 ↗ | (On Diff #66949) | Why the sizeof==8 rule? Good catch about the 1, slipped me, will drop change here. |
165 ↗ | (On Diff #66949) | By value on purpose. |
src/kmainwindow.cpp | ||
349 ↗ | (On Diff #66949) | dbusName and its chars ares modified here, |
src/kshortcutseditor.cpp | ||
715 ↗ | (On Diff #66949) | Wanted to make it a plain array, but forgot. |
src/kswitchlanguagedialog_p.cpp | ||
373 ↗ | (On Diff #66949) | members of languagesList are changed. |
src/ktoolbar.cpp | ||
187 ↗ | (On Diff #66949) | values detaches as before (where by use of non-const operator[]),now by internal use of begin()/end(), but this is fine, no? The "original" one is the class member values itself, so where do you see a non-original one? |
376 ↗ | (On Diff #66949) | Not directly related/needed to the actual change. Going for const arrays and making them constexpr I would do in a separate dedicated commit (with my having-had-to-read-lots-of.commit-history hat on :) ). |
update to @dhaumann's first review:
- drop wrong change for loop starting at 1
- use plain array for shortcutTitleToColumnMap