D10826: Introduce DocumentId class

[Michael Heidelbach]

michaelh added inline comments.

INLINE COMMENTS

> svuorela wrote in documentid.cpp:67
> 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..

This is intended. At the moment the class should behave like 
quint64(=DeviceIdAndInode)

> svuorela wrote in documentid.h:51-52
> it depends on why it is exported. If it is exported just for unit test (and 
> the header file not installed), then we don't need to have the mental and 
> code wise overhead of a d-pointer.
> 
> If we know that it will never ever need to grow, we also don't need the 
> overhead of a d-pointer. 
> So it is a big "it depends", which I was also kind of trying to ask into 
> earlier.

I have to admit, I have not understood the meaning of BALOO_ENGINE_EXPORT yet. 
The file which defines it looks like it's automatically created. I inserted it 
after trial and error. Except for testing I see no reason why it should be 
exported. I consider it as strictly internal.

In its present state this class shall serve as a replacement for the quint64 
that baloo is currently using for an id. 
Finally this class will have a payload of 24 bytes max. (64bit inode and 128bit 
uuid) and one method: `isValid()`. That at least is the plan.

This class is used a lot. What I have read about d-pointers so far indicates a 
huge performance hit when using indirection. Maybe I'm wrong.

@svuorela: I'm sorry if my answer appeared uncouth. That was not intended. 
Currently a lot of my programming is the result of trial and error rather than 
knowledge. In time the ratio, hopefully, will change in favor of the latter :)

REPOSITORY
  R293 Baloo

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

To: michaelh, adridg, #baloo, #frameworks
Cc: anthonyfieroni, svuorela, ashaposhnikov, michaelh, spoorun, nicolasfella, 
alexeymin

D10826: Introduce DocumentId class

[Sune Vuorela]

svuorela added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in documentid.h:51-52
> Make a d poniter in exported class.

it depends on why it is exported. If it is exported just for unit test (and the 
header file not installed), then we don't need to have the mental and code wise 
overhead of a d-pointer.

If we know that it will never ever need to grow, we also don't need the 
overhead of a d-pointer. 
So it is a big "it depends", which I was also kind of trying to ask into 
earlier.

REPOSITORY
  R293 Baloo

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

To: michaelh, adridg, #baloo, #frameworks
Cc: anthonyfieroni, svuorela, ashaposhnikov, michaelh, spoorun, nicolasfella, 
alexeymin

D10826: Introduce DocumentId class

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> documentid.h:51-52
> +private:
> +DeviceId m_device;
> +Inode m_inode;
> +};

Make a d poniter in exported class.

REPOSITORY
  R293 Baloo

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

To: michaelh, adridg, #baloo, #frameworks
Cc: anthonyfieroni, svuorela, ashaposhnikov, michaelh, spoorun, nicolasfella, 
alexeymin

D10826: Introduce DocumentId class

[Michael Heidelbach]

michaelh added a comment.


  I have made two errors:
  
  1. Forgot to submit the adapted tests
  2. An error in reasoning: I have based this and D10829 
 on a branch with the adapted tests  . As 
long as the changes are expected to be transparent it is much better to use the 
original tests. (This will also postpone the linker problems a little)

REPOSITORY
  R293 Baloo

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

To: michaelh, adridg, #baloo, #frameworks
Cc: svuorela, ashaposhnikov, michaelh, spoorun, nicolasfella, alexeymin

D10826: Introduce DocumentId class

[Michael Heidelbach]

michaelh added a comment.


  @svuorela: Thank you very much. I will try you suggestion as soon as I'm able 
to get the tests compiled.

INLINE COMMENTS

> svuorela wrote in documentid.h:34
> 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.

I got linker errors without this, so ... err yes.

REPOSITORY
  R293 Baloo

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

To: michaelh, adridg, #baloo, #frameworks
Cc: svuorela, ashaposhnikov, michaelh, kmorwinski, spoorun, nicolasfella, 
alexeymin

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

D10826: Introduce DocumentId class

[Michael Heidelbach]

michaelh added a dependent revision: D10829: Use DocumentId class.

REPOSITORY
  R293 Baloo

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

To: michaelh, adridg, #baloo, #frameworks
Cc: ashaposhnikov, michaelh, kmorwinski, spoorun, nicolasfella, alexeymin

D10826: Introduce DocumentId class

[Michael Heidelbach]

michaelh added a dependency: D10825: Introduce aliases DocId, DeviceId and 
Inode.

REPOSITORY
  R293 Baloo

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

To: michaelh, adridg, #baloo, #frameworks
Cc: ashaposhnikov, michaelh, kmorwinski, spoorun, nicolasfella, alexeymin

D10826: Introduce DocumentId class

[Michael Heidelbach]

michaelh created this revision.
michaelh added reviewers: adridg, Baloo, Frameworks.
michaelh added projects: Baloo, Frameworks.
michaelh requested review of this revision.

REVISION SUMMARY
  This class shall successively replace the current DocId(quint64) to gain more 
flexibility.
  
  - Account for 64bit inodes
  - Port away from stBuf.st_dev which is not always constant
  - Use document ids of variable size
  
  T8054 

TEST PLAN
  make test

REPOSITORY
  R293 Baloo

BRANCH
  flex-class (branched from flexible-docid)

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

AFFECTED FILES
  src/engine/documentid.cpp
  src/engine/documentid.h

To: michaelh, adridg, #baloo, #frameworks
Cc: ashaposhnikov, michaelh, kmorwinski, spoorun, nicolasfella, alexeymin