Default to QT_NO_CAST_FROM_ASCII/QT_NO_CAST_TO_ASCII/QT_NO_CAST_FROM_BYTEARRAY
ClosedPublic

Authored by kossebau on Aug 19 2018, 2:27 PM.

Diff Detail

Repository
R32 KDevelop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kossebau created this revision.Aug 19 2018, 2:27 PM
Restricted Application added a project: KDevelop. · View Herald TranscriptAug 19 2018, 2:27 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald Transcript
kossebau requested review of this revision.Aug 19 2018, 2:27 PM

There have been lots of commits across the whole codebase of all the plugins to use QLatin1String, QStringLiteral & Co.
Though the usage is not consistent, which leaves developer editing the code puzzled what to use.

Given that kdevelop is resource-heavy, default to the explicit classes with papercut-like performance effects might make sense in general, despite the more complex code to read for humans.

All plugins not yet building with the strict settings explicitly have to opt out, and would/could be ported one-by-one to follow the strict settings.

Test code should be always fine to not avoid the implicit conversions, so opt-out there would be always added.

kfunk requested changes to this revision.Aug 20 2018, 1:25 PM
kfunk added a subscriber: kfunk.

As discussed on IRC.

I'd prefer this being fixed right away instead of introducing so much (temporary) CMake code.

Clazy has auto fixits for these kind of issues: https://github.com/KDE/clazy/blob/master/docs/checks/README-qstring-allocations.md

This revision now requires changes to proceed.Aug 20 2018, 1:25 PM

As discussed on IRC.

I'd prefer this being fixed right away instead of introducing so much (temporary) CMake code.

Clazy has auto fixits for these kind of issues: https://github.com/KDE/clazy/blob/master/docs/checks/README-qstring-allocations.md

Tried that, but it seems most of the implicit casts were not detected, also many fixits were skipped due to strings being in macros and stuff. That needs more manual work :/

As discussed on IRC with Kevin, have now worked on fixing this right away, and this can go in now without all the temporary remove_definitions.
@volden Given you have mentioned you have a series of patches for the perforce plugin, I left that plugin out for now, so you do not run into conflicts. We would then complete the explicit-cast work here later :)

This revision was not accepted when it landed; it landed in state Needs Revision.Aug 24 2018, 8:18 PM
This revision was automatically updated to reflect the committed changes.

As discussed on IRC with Kevin, have now worked on fixing this right away, and this can go in now without all the temporary remove_definitions.
@volden Given you have mentioned you have a series of patches for the perforce plugin, I left that plugin out for now, so you do not run into conflicts. We would then complete the explicit-cast work here later :)

@kossebau I actually played around a little with the cast stuff myself. I now have it in a compiling/tests passing condition. Be happy to make a review request of that if you like.

As discussed on IRC with Kevin, have now worked on fixing this right away, and this can go in now without all the temporary remove_definitions.
@volden Given you have mentioned you have a series of patches for the perforce plugin, I left that plugin out for now, so you do not run into conflicts. We would then complete the explicit-cast work here later :)

@kossebau I actually played around a little with the cast stuff myself. I now have it in a compiling/tests passing condition. Be happy to make a review request of that if you like.

For the perforce plugin only that is, I should probably add. :-)