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

feat(Database): new databse for geode objects #535

Draft
wants to merge 19 commits into
base: master
Choose a base branch
from
Draft

Conversation

BotellaA
Copy link
Member

@BotellaA BotellaA commented Sep 3, 2022

No description provided.

src/geode/basic/database.cpp Outdated Show resolved Hide resolved
include/geode/basic/database.h Outdated Show resolved Hide resolved
src/geode/basic/database.cpp Outdated Show resolved Hide resolved
src/geode/basic/database.cpp Show resolved Hide resolved
src/geode/basic/database.cpp Outdated Show resolved Hide resolved
src/geode/basic/database.cpp Show resolved Hide resolved
tests/basic/test-database.cpp Outdated Show resolved Hide resolved
src/geode/basic/database.cpp Outdated Show resolved Hide resolved
src/geode/basic/database.cpp Show resolved Hide resolved
src/geode/basic/database.cpp Outdated Show resolved Hide resolved
src/geode/basic/database.cpp Show resolved Hide resolved
src/geode/basic/database.cpp Show resolved Hide resolved
tests/basic/test-database.cpp Show resolved Hide resolved
@BotellaA BotellaA changed the base branch from master to v13 December 8, 2022 14:09
@BotellaA
Copy link
Member Author

BotellaA commented Dec 9, 2022

We still need python binding and testing on more complex data like BRep. Should we do it now or later?
Database support for BRep and other could be done later as it is more full Bitsery support they need.

@BotellaA BotellaA marked this pull request as ready for review December 12, 2022 07:11
include/geode/basic/database.h Outdated Show resolved Hide resolved
Comment on lines 56 to 57
* @warning Do not destroy this Data class before the const reference
* obtained using its get() method is no longer in used
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @warning Do not destroy this Data class before the const reference
* obtained using its get() method is no longer in used
* @warning Do not destroy this Data class if the const reference
* obtained using its get() method is still used

else? what's happened if it is done?

Copy link
Member

Choose a reason for hiding this comment

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

Add a point at the end of the sentence

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we really need to document what happen if you di not respect the rule?

Copy link
Member

Choose a reason for hiding this comment

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

No but can you tell me, just to know for future debug?

Copy link
Member Author

Choose a reason for hiding this comment

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

when all the Data objects are deleted, the database starts a countdown to offload the actual data from the memory. So if you destroy your Data object but are still using the referenced data, you might get segfault or weird bugs because the memory has been cleaned.

* Retrieve a read only reference to the data corresponding to the given
* uuid.
*/
Data get_data( const uuid& id ) const;
Copy link
Member

Choose a reason for hiding this comment

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

read only but not const ref?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is a wrapper, not the real data

src/geode/basic/database.cpp Show resolved Hide resolved
Base automatically changed from v13 to master March 6, 2023 08:15
@BotellaA BotellaA marked this pull request as draft March 20, 2023 09:54
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.

3 participants