Details
Diff Detail
- Repository
- R318 Dolphin
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
| src/dolphinmainwindow.cpp | ||
|---|---|---|
| 413 | Because:
| |
| src/kitemviews/kitemlistcontroller.cpp | ||
| 436–437 | GCC doesn't not produce warning about fall through if it captures a comment about "fall through". By some reason GCC didn't like previous comment. I just made him happy. | |
If you request review from Dolphin and @elvisangelaccio comments, please wait for his "ship it".
| src/dolphintabwidget.cpp | ||
|---|---|---|
| 94–97 ↗ | (On Diff #28544) | Was this part of the original Diff people reviewed? |
| src/panels/information/phononwidget.cpp | ||
| 28–31 | Was this part of the original Diff people reviewed? | |
Could you not do this please?
The revision i reviewed was a lot different then the one that's up now. You even fix different things compared to what i first saw.
This should have been a new review i think, not an update.
| src/panels/information/phononwidget.cpp | ||
|---|---|---|
| 28–31 | Nope... | |
| 28–31 | -1 Those are GCC specific (what about clang and MSVC?). At least, that's my opinion, not sure about others. | |
| src/panels/information/phononwidget.cpp | ||
|---|---|---|
| 28–31 | I don't think that there is a cross-compiler way of ignoring warnings. In my opinion is better to make the code more ugly but to get rid of all warnings, because it's easier to spot a warning when it's only the one. | |
| src/panels/information/phononwidget.cpp | ||
|---|---|---|
| 28–31 | I usually tend to agree to changes that reduce compile warnings as i hate warnings too. In fact, adding override in phonon (to Phonon::VideoWidget::mouseMoveEvent and Phonon::VideoWidget::event would solve it, but it's probably not that easy). | |
Commit reverted, please be careful before using arc land.
| src/dolphintabwidget.cpp | ||
|---|---|---|
| 94–97 ↗ | (On Diff #28552) | Please remove. Warnings should be either fixed or hidden by a custom compiler config (you may want to try some cmake magic to override the gcc flags). Not in the code. |
| src/kitemviews/kitemlistcontroller.cpp | ||
|---|---|---|
| 436–437 | You're looking for Q_FALLTHROUGH() here I guess: http://doc.qt.io/qt-5/qtglobal.html#Q_FALLTHROUGH | |
| src/panels/information/phononwidget.cpp | ||
|---|---|---|
| 170 | This requires Qt 5.8, please bump QT_MIN_VERSION defined in the top-level CMakeLists.txt. | |
As Plasma itself depends on Qt 5.9, having Qt 5.8 in here as minimum is perfectly fine :)
It looks OK to me. Push it.
But please, no more changes on this change in phab afterwards. This review already changed way to wildly what should have been a new review request imho.