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

[Don't Merge, Meta one] Cleanup warnings #8684

Open
wants to merge 24 commits into
base: master
from

Conversation

@howard0su
Copy link
Contributor

howard0su commented Mar 22, 2020

Clean up warnings in various projects exclude the Externals folder.

Copy link
Member

lioncash left a comment

While I appreciate the gusto on trying to clean up some of these warnings, some of these changes seem to just silence warnings rather than properly fixing the underlying cause of said warnings. Particularly with ones that cast the destination type in memset calls to void*.

The commit messages also don't state what warnings were actually being resolved within them.

@@ -312,6 +312,7 @@ class PointerWrap
break;

case MODE_WRITE:
#pragma GCC diagnostic ignored "-Wmaybe-uninitialized"

This comment has been minimized.

Copy link
@lioncash

lioncash Mar 22, 2020

Member

This will not work on Windows and doesn't actually resolve the warning.

This comment has been minimized.

Copy link
@howard0su

howard0su Mar 22, 2020

Author Contributor

Sure. In order to clean up this, it may require significant rewrite of this file.

@@ -928,7 +928,7 @@ GCMemcardRemoveFileRetVal GCMemcard::RemoveFile(u8 index) // index in the direc
// here that has an empty file with the filename "Broken File000" where the actual deleted file
// was. Determine when exactly this happens and if this is neccessary for anything.

memset(&(UpdatedDir.m_dir_entries[index]), 0xFF, DENTRY_SIZE);
memset(static_cast<void*>(&(UpdatedDir.m_dir_entries[index])), 0xFF, DENTRY_SIZE);

This comment has been minimized.

Copy link
@lioncash

lioncash Mar 22, 2020

Member

This should probably not actually be silenced like this and instead have the initializtion and/or invalidation be done properly within a member function.

This comment has been minimized.

Copy link
@howard0su

howard0su Mar 22, 2020

Author Contributor

While I appreciate the gusto on trying to clean up some of these warnings, some of these changes seem to just silence warnings rather than properly fixing the underlying cause of said warnings. Particularly with ones that cast the destination type in memset calls to void*.

The commit messages also don't state what warnings were actually being resolved within them.

Your comments are fair. Let me split the patch and fix some places as you suggested.

Source/Core/Core/IOS/IOSC.cpp Outdated Show resolved Hide resolved
Source/Core/Core/PowerPC/JitCommon/JitCache.h Outdated Show resolved Hide resolved
Source/Core/Core/State.cpp Show resolved Hide resolved
@@ -56,14 +56,14 @@ ControlState AnalogStick::GetGateRadiusAtAngle(double ang) const
return m_stick_gate->GetRadiusAtAngle(ang);
}

OctagonAnalogStick::OctagonAnalogStick(const char* name, ControlState gate_radius)
: OctagonAnalogStick(name, name, gate_radius)
OctagonAnalogStick::OctagonAnalogStick(const char* _name, ControlState _gate_radius)

This comment has been minimized.

Copy link
@lioncash

lioncash Mar 22, 2020

Member

We prefer to prefix names with the underscore after the identifier like so: name_

Ditto for all other occurrences

This comment has been minimized.

Copy link
@howard0su

howard0su Mar 22, 2020

Author Contributor

I use this style because in ControlGroup subfolder, there is exiting code like this. If you prefer other way, I can submit a seperate patch to cleanup them.

This comment has been minimized.

Copy link
@BhaaLseN

BhaaLseN Mar 22, 2020

Member

Why did you have to change this anyways? Members should have the m_ prefix already and not cause any shadowing with the parameters.
In case I did miss some, use a suffix as lioncash suggested.

This comment has been minimized.

Copy link
@howard0su

howard0su Mar 22, 2020

Author Contributor

sure. I can make a big change to add a prefix m_ to the fields in the class of ControlGroup

This comment has been minimized.

Copy link
@BhaaLseN

BhaaLseN Mar 22, 2020

Member

Please don't. I only see name being used there, so I don't quite see the need to rename the others (like gate_radius) though?

This comment has been minimized.

Copy link
@howard0su

howard0su Mar 22, 2020

Author Contributor

so only change name and ui_name here?

This comment has been minimized.

Copy link
@BhaaLseN

BhaaLseN Mar 22, 2020

Member

Only the ones necessary to resolve the shadowing.

Source/Core/VideoBackends/Software/Rasterizer.cpp Outdated Show resolved Hide resolved
@@ -43,7 +43,7 @@ static const float s_gammaLUT[] = {1.0f, 1.7f, 2.2f, 1.0f};

void BPInit()
{
memset(&bpmem, 0, sizeof(bpmem));
memset(static_cast<void*>(&bpmem), 0, sizeof(bpmem));

This comment has been minimized.

Copy link
@lioncash

lioncash Mar 22, 2020

Member

This is, again, hiding an issue rather than taking the time to fix it properly.

This comment has been minimized.

Copy link
@howard0su

howard0su Mar 22, 2020

Author Contributor

okay. I will fix it.

Source/Core/VideoCommon/ShaderCache.cpp Outdated Show resolved Hide resolved
Source/Core/VideoCommon/VideoBackendBase.cpp Outdated Show resolved Hide resolved
howard0su added 3 commits Mar 22, 2020
howard0su added 3 commits Mar 22, 2020
This memset is not needed. All the fields are filled in the next lines.
@howard0su

This comment has been minimized.

Copy link
Contributor Author

howard0su commented Mar 22, 2020

Maybe it is easy for me to split this change into smaller ones.

Source/Core/VideoCommon/CPMemory.h Outdated Show resolved Hide resolved
Source/Core/VideoCommon/CPMemory.h Outdated Show resolved Hide resolved
Source/Core/VideoCommon/VideoBackendBase.cpp Outdated Show resolved Hide resolved
howard0su added 2 commits Mar 23, 2020
turns out compiler will generate the code by default.
@howard0su howard0su changed the title Cleanup warnings [Don't Merge, Meta one] Cleanup warnings Mar 23, 2020
@howard0su

This comment has been minimized.

Copy link
Contributor Author

howard0su commented Mar 23, 2020

start splitting this PR. Keep this open for comments. I split the code already reviewed into new PRs. please help check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.