GIT_SILENT
Details
Details
Test pass, KXMLGUI-based apps showed no regressions incl. editing of
toolbars & shortcuts
Diff Detail
Diff Detail
- Repository
- R263 KXmlGui
- Branch
- morerangebasedforloops
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 17160 Build 17178: arc lint + arc unit
| src/kedittoolbar.cpp | ||
|---|---|---|
| 1566–1567 | No qAsConst? If done by intention, maybe keep the iterator version? | |
| src/kgesture.cpp | ||
| 117–119 | qAsConst? | |
| 133 |
| |
| 165–167 | QPoint & | |
| src/kmainwindow.cpp | ||
| 349–351 | qAsConst? | |
| src/kshortcutseditor.cpp | ||
| 715–720 | Maybe make it a QVector. Or simply an initializer list. | |
| src/kswitchlanguagedialog_p.cpp | ||
| 373–374 | qAsConst | |
| src/ktoolbar.cpp | ||
| 187–188 | Let's say values detaches here, then the changes value doesn't have any effect on the original one. | |
| 376 | constexpr? | |
Comment Actions
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–1567 | By intention, as the elements of the vector are modified. | |
| src/kgesture.cpp | ||
| 117–119 | Again, members of d->m_shape are changed (thus also the QPoint&) | |
| 133 | Why the sizeof==8 rule? Good catch about the 1, slipped me, will drop change here. | |
| 165–167 | By value on purpose. | |
| src/kmainwindow.cpp | ||
| 349–351 | dbusName and its chars ares modified here, | |
| src/kshortcutseditor.cpp | ||
| 715–720 | Wanted to make it a plain array, but forgot. | |
| src/kswitchlanguagedialog_p.cpp | ||
| 373–374 | members of languagesList are changed. | |
| src/ktoolbar.cpp | ||
| 187–188 | 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 | 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 :) ). | |
Comment Actions
update to @dhaumann's first review:
- drop wrong change for loop starting at 1
- use plain array for shortcutTitleToColumnMap