From kde-devel Wed Aug 30 18:08:16 2017 From: Christian Ehrlicher Date: Wed, 30 Aug 2017 18:08:16 +0000 To: kde-devel Subject: Re: Review Request 129349: [baloo] add (and use) FileIndexerConfig::shouldFileBeIndexed(const QStrin Message-Id: <20170830180816.1801.16777 () mimi ! kde ! org> X-MARC-Message: https://marc.info/?l=kde-devel&m=150411653311626 --===============8027707297312679331== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit > On Aug. 29, 2017, 9:03 nachm., Kai Uwe Broulik wrote: > > src/file/fileindexerconfig.cpp, line 173 > > > > > > You sure this doesn't crash? I've seen crashes when you directly chain a Ref to another method. > > Stefan Brüns wrote: > Where do you see a chained ref? I don't see a problem here. QString::midRef() returns a reference to path. QStringRef::mid() works on this reference and once again returns refernces to path. Since path is still alive then, all is fine. If this should somehow crash it's a but in Qt... - Christian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129349/#review103663 ----------------------------------------------------------- On Aug. 24, 2017, 5:48 vorm., Christian Ehrlicher wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/129349/ > ----------------------------------------------------------- > > (Updated Aug. 24, 2017, 5:48 vorm.) > > > Review request for Baloo. > > > Repository: baloo > > > Description > ------- > > Use the new QStringRef class to avoid creation of temporary QString objects. > Since every filename is sent through FileIndexerConfig::shouldFileBeIndexed(), I think this is a good optimization although now there are two functions for FileIndexerConfig::shouldFileBeIndexed() and RegExpCache::exactMatch() - one with QString and one with QStringRef ... > > > Diffs > ----- > > src/engine/queryparser.cpp 9c3a4b60 > src/file/fileindexerconfig.h e2466bdd > src/file/fileindexerconfig.cpp 0e4f4c45 > src/file/modifiedfileindexer.cpp d1f5011b > src/file/newfileindexer.cpp 7e196229 > src/file/regexpcache.h 60d370de > src/file/regexpcache.cpp 61f07d61 > src/file/xattrindexer.cpp 3daecef6 > src/kioslaves/timeline/timelinetools.cpp 26e44ee8 > > Diff: https://git.reviewboard.kde.org/r/129349/diff/ > > > Testing > ------- > > > Thanks, > > Christian Ehrlicher > > --===============8027707297312679331== MIME-Version: 1.0 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 8bit
This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129349/

On August 29th, 2017, 9:03 nachm. UTC, Kai Uwe Broulik wrote:

src/file/fileindexerconfig.cpp (Diff revisions 3 - 4)
173 
        const QVector<QStringRef> pathComponents = path.mid(folder.count()).splitRef(QLatin1Char('/'), QString::SkipEmptyParts);
 173 
        const QVector<QStringRef> pathComponents = path.midRef(folder.count()).split(QLatin1Char('/'), QString::SkipEmptyParts);
 
You sure this doesn't crash? I've seen crashes when you directly chain a Ref to another method.

On August 30th, 2017, 3:51 nachm. UTC, Stefan Brüns wrote:

Where do you see a chained ref?
I don't see a problem here. QString::midRef() returns a reference to path. QStringRef::mid() works on this reference and once again returns refernces to path. Since path is still alive then, all is fine. If this should somehow crash it's a but in Qt...

- Christian


On August 24th, 2017, 5:48 vorm. UTC, Christian Ehrlicher wrote:

Review request for Baloo.
By Christian Ehrlicher.

Updated Aug. 24, 2017, 5:48 vorm.

Repository: baloo

Description 

Use the new QStringRef class to avoid creation of temporary QString objects.
Since every filename is sent through FileIndexerConfig::shouldFileBeIndexed(), I think this is a good optimization although now there are two functions for FileIndexerConfig::shouldFileBeIndexed() and RegExpCache::exactMatch() - one with QString and one with QStringRef ...

Diffs

  • src/engine/queryparser.cpp (9c3a4b60)
  • src/file/fileindexerconfig.h (e2466bdd)
  • src/file/fileindexerconfig.cpp (0e4f4c45)
  • src/file/modifiedfileindexer.cpp (d1f5011b)
  • src/file/newfileindexer.cpp (7e196229)
  • src/file/regexpcache.h (60d370de)
  • src/file/regexpcache.cpp (61f07d61)
  • src/file/xattrindexer.cpp (3daecef6)
  • src/kioslaves/timeline/timelinetools.cpp (26e44ee8)

View Diff
--===============8027707297312679331==--