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

Reduce number of warnings #179

Closed

Conversation

MadVitaliy
Copy link
Contributor

Fix of: -Wclass-memaccess -Wuse-after-free -Werror=unused-but-set-variable -Werror=unused-variable -Werror=nonnull for GCC 12.
This PR is updated #177.

@@ -45,7 +45,7 @@ class CTable

CTable()
{
::memset(_rgtype, 0, sizeof(_rgtype));
::memset((void*)_rgtype, 0, sizeof(_rgtype));
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, this isn't fixing anything, it's just suppressing the warning... and the warning looks correct to me.

Comment on lines 40 to 48
{
ForkProcess* cur = Fork::ForkProcess::list;
while(cur != nullptr){
ForkProcess* next = cur->next;
if(cur->kill_child)
delete cur;
break;
}

cur = next;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The commit message doesn't say what the change is about...

@@ -607,7 +607,7 @@ class Element<TVR, VM::VM1_n>
Save = true; // ????
if( Internal )
{
memcpy(internal, Internal, len);
memcpy((void*)internal, (void*)Internal, Length);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here. This is suppressing a warning, not fixing anything...

Comment on lines 38 to 46
// First, kill all children whose kill_child flag is set.
// Second, wait for other children to die.
{
for (ForkProcess* cur = Fork::ForkProcess::list; cur; cur = cur->next)
if (cur->kill_child)

for (ForkProcess* cur = Fork::ForkProcess::list; cur != nullptr; cur = cur->next)
if (cur->kill_child) {
delete cur;
break;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment above this code says "kill all children" so breaking after killing one seems wrong...

@@ -58,7 +58,7 @@ unique_ptr<JpegMarkerSegment> JpegMarkerSegment::CreateJpegFileInterchangeFormat
content.push_back(static_cast<uint8_t>(params.Ythumbnail));
if (params.Xthumbnail > 0)
{
if (params.thumbnail)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is 3rd party code, so needs separate handling.

Is this reversal of logic fixed upstream already perhaps?

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 am not sure if it's 3rd party code but may be a usage of 3rd party library.
Also, it does not seem to be a reversal of logic. This code sample was edited 8 years ago.

I made this change to align logic with the following exception message which is ""params.Xthumbnail is > 0 but params.thumbnail == null_ptr""

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if it's 3rd party code but may be a usage of 3rd party library.

It definitely is: https://github.com/team-charls/charls

@malaterre
Copy link
Owner

@MadVitaliy I've cherry-picked the two commits that were clear to me. Feel free to rebase and re-submitted with extended description. As explained third party update should not be addressed like this. You need to pull in the full version even in the case of a single line fix. Thanks !

@malaterre malaterre closed this Jan 7, 2025
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