--===============2062187513419871557== Content-Type: multipart/alternative; boundary="===============4707144253455356749==" --===============4707144253455356749== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112248/#review38513 ----------------------------------------------------------- Sorry, I am against this one because I think it considerably deteriorates code readability for very little performance gain. Frank, most of the checks on EBN are for micro-optimizations. They are low hanging fruit, and as such they generally make sense to follow, but there are corner cases like this one where the cost outweighs the benefits. - Mark Kretschmann On Aug. 24, 2013, 5:32 p.m., Frank Meerkoetter wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/112248/ > ----------------------------------------------------------- > > (Updated Aug. 24, 2013, 5:32 p.m.) > > > Review request for Amarok. > > > Description > ------- > > QLatin1String is more efficient. > > http://www.englishbreakfastnetwork.org/krazy/reports/extragear/multimedia/amarok/index.html > [...] > 33: Check for strings used improperly or should be i18n. [strings]...OOPS! 78 issues found! > [...] > > > This addresses bug 313504. > https://bugs.kde.org/show_bug.cgi?id=313504 > > > Diffs > ----- > > shared/tag_helpers/TagHelper.cpp 7528a77 > src/MediaDeviceCache.cpp 73e8f34 > src/ScriptManager.cpp 4b4fc14 > src/amarokurls/NavigationUrlGenerator.cpp 0456976 > src/amarokurls/NavigationUrlRunner.cpp bde18e5 > src/context/applets/info/InfoApplet.cpp 760b241 > src/context/engines/similarartists/SimilarArtistsEngine.cpp 898beb4 > src/context/engines/tabs/TabsEngine.cpp 24a89df > src/core-impl/collections/audiocd/AudioCdCollection.cpp 3dfa7c3 > src/core-impl/collections/daap/DaapCollection.cpp 20e7b4f > src/core-impl/collections/db/sql/SqlMeta.cpp b399ed3 > src/core-impl/collections/ipodcollection/IpodCollectionFactory.cpp 7ecdcfa > src/core-impl/collections/support/ArtistHelper.cpp 4c3f742 > src/core-impl/collections/umscollection/UmsCollection.cpp ad38461 > src/core-impl/collections/upnpcollection/UpnpBrowseCollection.cpp 6467964 > src/core-impl/collections/upnpcollection/UpnpCollectionFactory.cpp 80ea8de > src/core-impl/collections/upnpcollection/UpnpMeta.cpp 9b660de > src/core-impl/meta/cue/CueFileSupport.cpp 77d27e6 > src/core-impl/meta/file/File.cpp f2abea4 > src/core-impl/playlists/types/file/m3u/M3UPlaylist.cpp 28e3b00 > src/core/support/Amarok.cpp ca59725 > src/dialogs/CollectionSetup.cpp 0672421 > src/services/amazon/AmazonStore.cpp eb42566 > src/services/ampache/AmpacheAccountLogin.cpp 26aebed > src/services/ampache/AmpacheServiceQueryMaker.cpp b94c3b0 > src/services/lastfm/ScrobblerAdapter.cpp 3c81546 > src/services/magnatune/MagnatuneSqlCollection.cpp 2f50e90 > src/services/mp3tunes/Mp3tunesWorkers.cpp a7ebd36 > src/statsyncing/collection/CollectionProvider.cpp a26cec6 > src/widgets/Osd.cpp 32c46e9 > supplementary_scripts/licenseChecker/log.cpp ba9b0fd > supplementary_scripts/licenseChecker/main.cpp 4e6bba8 > tests/core-impl/collections/db/sql/TestSqlCollectionLocation.cpp ab41db3 > tests/core-impl/support/TestTrackLoader.cpp ce369e2 > utilities/afttagger/AFTTagger.cpp 5a60c6b > utilities/amzdownloader/AmzDownloader.cpp e6771de > utilities/collectionscanner/CollectionScanner.cpp dd4ae37 > > Diff: http://git.reviewboard.kde.org/r/112248/diff/ > > > Testing > ------- > > Unit tests pass. > > > Thanks, > > Frank Meerkoetter > > --===============4707144253455356749== Content-Type: text/html; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit
This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112248/ 

Sorry, I am against this one because I think it considerably deteriorates code readability for very little performance gain.

Frank, most of the checks on EBN are for micro-optimizations. They are low hanging fruit, and as such they generally make sense to follow, but there are corner cases like this one where the cost outweighs the benefits.

- Mark


On August 24th, 2013, 5:32 p.m. UTC, Frank Meerkoetter wrote:

Review request for Amarok.
By Frank Meerkoetter.

Updated Aug. 24, 2013, 5:32 p.m.

Description 

QLatin1String is more efficient.

http://www.englishbreakfastnetwork.org/krazy/reports/extragear/multimedia/amarok/index.html
[...]
33: Check for strings used improperly or should be i18n. [strings]...OOPS! 78 issues found!
[...]

Testing 

Unit tests pass.
Bugs: 313504

Diffs

  • shared/tag_helpers/TagHelper.cpp (7528a77)
  • src/MediaDeviceCache.cpp (73e8f34)
  • src/ScriptManager.cpp (4b4fc14)
  • src/amarokurls/NavigationUrlGenerator.cpp (0456976)
  • src/amarokurls/NavigationUrlRunner.cpp (bde18e5)
  • src/context/applets/info/InfoApplet.cpp (760b241)
  • src/context/engines/similarartists/SimilarArtistsEngine.cpp (898beb4)
  • src/context/engines/tabs/TabsEngine.cpp (24a89df)
  • src/core-impl/collections/audiocd/AudioCdCollection.cpp (3dfa7c3)
  • src/core-impl/collections/daap/DaapCollection.cpp (20e7b4f)
  • src/core-impl/collections/db/sql/SqlMeta.cpp (b399ed3)
  • src/core-impl/collections/ipodcollection/IpodCollectionFactory.cpp (7ecdcfa)
  • src/core-impl/collections/support/ArtistHelper.cpp (4c3f742)
  • src/core-impl/collections/umscollection/UmsCollection.cpp (ad38461)
  • src/core-impl/collections/upnpcollection/UpnpBrowseCollection.cpp (6467964)
  • src/core-impl/collections/upnpcollection/UpnpCollectionFactory.cpp (80ea8de)
  • src/core-impl/collections/upnpcollection/UpnpMeta.cpp (9b660de)
  • src/core-impl/meta/cue/CueFileSupport.cpp (77d27e6)
  • src/core-impl/meta/file/File.cpp (f2abea4)
  • src/core-impl/playlists/types/file/m3u/M3UPlaylist.cpp (28e3b00)
  • src/core/support/Amarok.cpp (ca59725)
  • src/dialogs/CollectionSetup.cpp (0672421)
  • src/services/amazon/AmazonStore.cpp (eb42566)
  • src/services/ampache/AmpacheAccountLogin.cpp (26aebed)
  • src/services/ampache/AmpacheServiceQueryMaker.cpp (b94c3b0)
  • src/services/lastfm/ScrobblerAdapter.cpp (3c81546)
  • src/services/magnatune/MagnatuneSqlCollection.cpp (2f50e90)
  • src/services/mp3tunes/Mp3tunesWorkers.cpp (a7ebd36)
  • src/statsyncing/collection/CollectionProvider.cpp (a26cec6)
  • src/widgets/Osd.cpp (32c46e9)
  • supplementary_scripts/licenseChecker/log.cpp (ba9b0fd)
  • supplementary_scripts/licenseChecker/main.cpp (4e6bba8)
  • tests/core-impl/collections/db/sql/TestSqlCollectionLocation.cpp (ab41db3)
  • tests/core-impl/support/TestTrackLoader.cpp (ce369e2)
  • utilities/afttagger/AFTTagger.cpp (5a60c6b)
  • utilities/amzdownloader/AmzDownloader.cpp (e6771de)
  • utilities/collectionscanner/CollectionScanner.cpp (dd4ae37)

View Diff
--===============4707144253455356749==-- --===============2062187513419871557== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel --===============2062187513419871557==--