D8588: Make VCS data container classes implicitly shared
Friedrich W. H. Kossebau
noreply at phabricator.kde.org
Thu Nov 2 17:28:47 UTC 2017
kossebau added a comment.
In https://phabricator.kde.org/D8588#163592, @mwolff wrote:
> 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.
To: kossebau, #kdevelop
Cc: mwolff, brauch, kdevelop-devel
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the KDevelop-devel