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

Fix error in User_Memory_Init_Pools #1109

Merged
merged 2 commits into from
Mar 8, 2024

Conversation

xezon
Copy link
Contributor

@xezon xezon commented Feb 11, 2024

This claims to be a fix for a Thyme specific issue.

@codecov-commenter
Copy link

codecov-commenter commented Feb 11, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (64626cd) 2.54% compared to head (04d1fc5) 2.54%.

Files Patch % Lines
src/game/common/system/gamememoryinit.cpp 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1109      +/-   ##
===========================================
- Coverage     2.54%    2.54%   -0.01%     
===========================================
  Files          949      949              
  Lines       110299   110300       +1     
  Branches     18881    18881              
===========================================
  Hits          2802     2802              
- Misses      107094   107095       +1     
  Partials       403      403              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -592,6 +592,7 @@ bool DDSFileClass::Load()

if (data == 0) {
captainslog_warn("DDSFile '%s' appears to contain no data, size is %d.", m_name, fp->Size());
fp->Close();
Copy link
Contributor

Choose a reason for hiding this comment

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

auto_file_ptr should indirectly be doing that?

Copy link
Contributor

Choose a reason for hiding this comment

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

No it does not.

Copy link
Contributor

Choose a reason for hiding this comment

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

right, comment as bugfix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps the right fix here is then to auto close on destruction of auto_file_ptr instead.

I will take a look if there are more places where auto_file_ptr is missing call to Close.

Copy link
Contributor

Choose a reason for hiding this comment

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

Original auto_file_ptr didn't call Close, we should keep it that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have reverted change to DDSFileClass::Load and will make a new Pull for auto_file_ptr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have taken another look at auto_file_ptr and GameFileClass in debugger. It does call Close on destruction via virtual destructor:

GameFileClass::~GameFileClass()
{
    Close();
}

So this needs no change.

@xezon xezon force-pushed the fix-error-ddsfileclass-load branch from e1209f8 to 04d1fc5 Compare February 16, 2024 17:57
@xezon xezon changed the title Fix errors in DDSFileClass::Load, User_Memory_Init_Pools Fix error in User_Memory_Init_Pools Feb 16, 2024
@xezon xezon force-pushed the fix-error-ddsfileclass-load branch from 04d1fc5 to 1105b4e Compare March 6, 2024 18:23
@xezon xezon merged commit 2658e67 into TheAssemblyArmada:develop Mar 8, 2024
7 checks passed
@xezon xezon deleted the fix-error-ddsfileclass-load branch March 8, 2024 14:47
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.

4 participants