Make VCS data container classes implicitly shared
ClosedPublic

Authored by kossebau on Nov 1 2017, 9:33 AM.

Details

Summary

Also

  • rely on default implementation of copy constructor to copy all data on forking the internal data objects
  • mark as movable types for improved handling in Qt containers

Object of these classes are created in one place and then passed around
for read-only consumption. They are passed by value in method return
arguments and stored by value. As the classes already have a private
object for pimpl, making this an implicitly shared one is a small step.

Diff Detail

Repository
R32 KDevelop
Branch
makedataobjectimplicitelyshared
Lint
No Linters Available
Unit
No Unit Test Coverage
kossebau created this revision.Nov 1 2017, 9:33 AM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptNov 1 2017, 9:33 AM

VcsLocation would be another candidate, but that one has some puzzling non-const operator== overload with different comparison behaviour, which needs some more investigation to understand that class intention.

BTW, if anyone could tell how to reduce all the code duplication in the tests for checking the behaviour per property, recommendations welcome :)

brauch added a subscriber: brauch.Nov 1 2017, 10:05 AM

In general looks like a good idea and I admire your persistence, but do we really want 3k lines of code of tests testing copy constructors? Does that add value? It seems like it just makes any future changes harder ... It only tests that the compiler-generated code actually does what the compiler claims it will do, no? ;)

kossebau added a comment.EditedNov 1 2017, 10:34 AM

In general looks like a good idea and I admire your persistence, but do we really want 3k lines of code of tests testing copy constructors?

Yes, result smells a little insane. Story behind is that I wanted to be a good boy(tm) and cover the changes done by some unit tests. Should be simple, right? But suddenly my memory, notes and accessible code had no templates for testing normal C++ properties API, so I turned to just experimenting.
I hoped by writing out all the code I could see some patterns which could be used to fold things into a clever use of loops, functors or lambdas. But I failed, any tricks I tried did not really reduce the total number of lines, but added complexity.
So at that point I gave up and just kept the latest version as proposed here :)

Does that add value? It seems like it just makes any future changes harder ... It only tests that the compiler-generated code actually does what the compiler claims it will do, no? ;)

(Somehow I mapped this onto "Unit tests making future changes harder" on first reading, how comes ;) ) Actually I tested with the tests that old and new implementation does the same, especially as I was unsure what the old implementation should behave like (incl. bugs which are used as "feature") given the API dox had no promises. And then they say ideally unit tests should not know about the implementation, but simply check the API promises?

Another thing to consider: those classes have been stable for many years, so if someone ever decides to change things in their API, they still could dump these unit tests.
The test code is here now, seems to work and does not have much testtime costs (< 1s here).
So IMHO the small value still > small costs.

kossebau updated this revision to Diff 21695.Nov 1 2017, 1:27 PM

simplify unit tests a little by testing detaching only by one property

After all we know details of the implementation, and as we test specifically
for the detach situation where we know it should be the same for any
non-const method, doing just one setter and then testing the values of all
properties in both copies should cover the most potential errors.

-> less code, more coverage of critical behaviour

mwolff added a subscriber: mwolff.Nov 2 2017, 4:17 PM

Big change is big. Why did you bother doing it? Do you have performance measurements indicating that this is a bottleneck? If so, do you want to save memory or do you want to make things faster? Would it maybe be enough to add move semantics?

I'm not in principle opposed to this patch, considering the work is done. Though for the future, please try to trim down the verbosity of the tests. They are really much too big. I.e. testing the permutations of changing A and then changing B is pretty pointless imo.

Big change is big. Why did you bother doing it? Do you have performance measurements indicating that this is a bottleneck? If so, do you want to save memory or do you want to make things faster?

Motivation was: had seen quite some vcs kdevplatform code & API usage recently, and when noticing in code those classes do deep copies every time, it seemed they should become shared data classes rather (especially as that change was just a few lines to do and done in a few minutes, other than the hours spent on experimenting with the unit tests).

Would it maybe be enough to add move semantics?

No, as the lifetime is: each item generated as part of group of items representing VCS data read in, then being kept around for read-only access in containers. Until there move semantics would work I guess,. But the items in the containers are then also partially accessed via a querying API that returns the items by value. Where then shared data seems a better approach over deep copies.

But in only-do-changes-after-measurement class I deserve a bad grade, no single measurement done. The new implementation just "felt" better by principles.

I'm not in principle opposed to this patch, considering the work is done. Though for the future, please try to trim down the verbosity of the tests. They are really much too big. I.e. testing the permutations of changing A and then changing B is pretty pointless imo.

Okay, could drop the B ones, more a left-over from my experiments how to do a full coverage, agreed it does not add much knowing the implementation. Will update later tonight.

kossebau updated this revision to Diff 21813.Nov 2 2017, 10:17 PM

drop variant of testing detach of B from tests

apol accepted this revision.Nov 3 2017, 12:09 AM
apol added a subscriber: apol.

I agree this is better.

Let's be more liberal with reviews? I just saw 2 reviews whose end was to remove tests.

Let's agree that verbosity in tests isn't the worst in life, we have _very_ bard code in KDevelop, these tests aren't close by far.
And thanks @kossebau for taking the time to take our codebase steps forward little by little.

This revision is now accepted and ready to land.Nov 3 2017, 12:09 AM
In D8588#163801, @apol wrote:

And thanks @kossebau for taking the time to take our codebase steps forward little by little.

:)

@mwolff @brauch So any further objection to get this in as is now?

BTW, am still interested in some pointers for best practices of testing such data container classes, if anyone has some to share.

mwolff accepted this revision.Nov 3 2017, 10:17 AM

lgtm. and to be clear: better verbose tests than no tests. so thanks! when i have time i'll try to cleanup the tests a bit to show how i think one could reduce the verbosity

No, no objections, it doesn't really do any harm. Thanks!

This revision was automatically updated to reflect the committed changes.