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

[no sq] Server bug fixes and unit tests #14423

Merged
merged 5 commits into from Mar 17, 2024
Merged

Conversation

sfan5
Copy link
Member

@sfan5 sfan5 commented Mar 2, 2024

To do

This PR is a Work in Progress / Ready for Review.

How to test

@grorp
Copy link
Member

grorp commented Mar 3, 2024

It would be very nice if you could add unittests for c566ffe and 8e7f3b9. The behavior seems easy to mess up and (at least in parts) well testable.

@sfan5
Copy link
Member Author

sfan5 commented Mar 3, 2024

I'll check how easily I can test it from C++. Devtest is challenging due to missing direct control and insight into this part.

@rubenwardy

This comment was marked as resolved.

@sfan5 sfan5 changed the title [no sq] Misc bug fixes [no sq] Server bug fixes and unit tests Mar 3, 2024
@sfan5
Copy link
Member Author

sfan5 commented Mar 3, 2024

@grorp Tests added.

delete m_thread;
// Note that this also deletes and saves the map.
delete m_env;
m_env = nullptr;
Copy link
Member

Choose a reason for hiding this comment

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

setting m_env to nullptr in destructor means we have a scope issue somewhere in the destruction. Which code part uses m_env after that and will break ? (it means we should find a wait to split some objects at a moment)

Copy link
Member Author

Choose a reason for hiding this comment

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

this is just good practice to not leave dangling pointers around


// Activate object
if (object->m_static_exists)
{
Copy link
Member

Choose a reason for hiding this comment

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

wrong brace :)

Copy link
Member Author

Choose a reason for hiding this comment

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

intentional so I can properly place the comment before else, suggestions welcome

src/serverenvironment.cpp Show resolved Hide resolved
@sfan5 sfan5 added the Rebase needed The PR needs to be rebased by its author. label Mar 16, 2024
Copy link
Member

@Desour Desour left a comment

Choose a reason for hiding this comment

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

I'm not too familiar with static object stuff. But code looks sane.

@sfan5 sfan5 removed the Rebase needed The PR needs to be rebased by its author. label Mar 17, 2024
@sfan5 sfan5 merged commit e3b9828 into minetest:master Mar 17, 2024
15 checks passed
@sfan5 sfan5 deleted the branch0103 branch March 17, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Objects with static_save = false are persisted
5 participants