D10826: Introduce DocumentId class

[Sune Vuorela]

svuorela added a comment.


  A quick review of some quirks and weirdnesses in c++

INLINE COMMENTS

> documentid.cpp:67
> +#if(0)
> +// Due to operator DeviceIdAndInode(), apparently the following
> +// became obsolete.

if you make operator DeviceIdAndInode explicit, you probably end up with a bit better behavior.

Else the compiler happily converts a documentid to a deviceidandinode in order to make various comparisons at compile time.

e.g. DocumentId id(5,5);
bool b = true;
if (id == b) { ... }  should compile, but probably isn't intended behavior..

> documentid.h:34
> +
> +class BALOO_ENGINE_EXPORT DocumentId
> +{

Does it need to be exported? Is things outside of the baloo engine need to access this? (or is it just avaliable for tests)

If it is public-public, you need to be careful about the class size and ensure you have enough bits to work with in the members.

> documentid.h:41
> +    //TODO: DocumentId(const QT_STATBUF& stBuf);
> +    ~DocumentId();
> +    

is this needed? or is the default generated good enough?

> documentid.h:45
> +    
> +    DocumentId operator=(const DeviceIdAndInode devino);
> +    DocumentId operator=(const DocumentId& id);

shouldn't this return a DocumentId& rather than a copy?

> documentid.h:46
> +    DocumentId operator=(const DeviceIdAndInode devino);
> +    DocumentId operator=(const DocumentId& id);
> +    

=default

  or if you remove the custom dtor, this will be autogenerated.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D10826

To: michaelh, adridg, #baloo, #frameworks
Cc: svuorela, ashaposhnikov, michaelh, kmorwinski, spoorun, nicolasfella, alexeymin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20180225/3c3146a6/attachment.html>