Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement implicit sharing for QgsField #2019

Closed
wants to merge 2 commits into from

Conversation

nyalldawson
Copy link
Collaborator

Implements implicit sharing for QgsField. I'm seeking comments on whether this is a correct implementation of Qt's implicit sharing classes, and if so, will continue to switch additional classes to be implicitly shared.

@@ -86,9 +88,9 @@ public:

/**
Set the field type.
@param typ Field type
@param type Field type
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typeName

@m-kuhn
Copy link
Member

m-kuhn commented May 1, 2015

If you want to experiment with implicit sharing, go for QgsFeature, it would be great to be able to copy this around without having to care much :)

@nyalldawson
Copy link
Collaborator Author

@m-kuhn agreed, but I wanted to start with something straightforward as a trial...

@m-kuhn
Copy link
Member

m-kuhn commented May 1, 2015

@nyalldawson fair enough.
Concerning the implementation, is there no need to provide code for the operator=() method?

@nyalldawson
Copy link
Collaborator Author

@m-kuhn I think QSharedData/QSharedDataPointer takes care of that automatically. I'm struggling to find definite answers either way...

@m-kuhn
Copy link
Member

m-kuhn commented May 1, 2015

I think it may be good to define FieldData in a separate header file that is not considered public API. This should reduce compile time and maybe even runtime loading performance. If I am not mistaken we should be able to profit from that: https://gcc.gnu.org/wiki/Visibility (at least our windows builds as I think we don't use visibility stuff on other platforms)

PS: Please don't expect that I completely have understood the article in every detail...

@nyalldawson
Copy link
Collaborator Author

@m-kuhn how's this? It probably needs some extra cmake changes to keep the _p.h header private too...

@m-kuhn
Copy link
Member

m-kuhn commented May 2, 2015

Looks good to me. Maybe the private header could somehow be removed from install/devel-packages. But I don't think it hurts apart from inflating them.

@nyalldawson
Copy link
Collaborator Author

manually merged

@nyalldawson nyalldawson closed this May 2, 2015
@wonder-sk
Copy link
Member

Great stuff :-)

elpaso added a commit to elpaso/QGIS that referenced this pull request Oct 23, 2018
… caller

Fix qgis#2019 - DBManager fails to display error messages with virtual layers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants