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

Add -Wshadow=local to warnings and fix reported issues (#25316). #25853

Merged
merged 1 commit into from
Feb 20, 2019

Conversation

marxin
Copy link
Contributor

@marxin marxin commented Feb 13, 2019

I've made a mechanical changes in order to silent the warning.
I'm also attaching a script that I used to compare that objdump -S is identical (objdump-S.diff).
Moreover, I checked that .roda and .data are equal mudulo git revision and new assert strings (objdump-data-rodata.diff), it's quite nicely seen in output of strings command (strings.diff).
Please tell me what you think about the changes?

difference.tar.gz

@marxin marxin changed the title Add -Wshadow=local to warnings and fix reported issues. Add -Wshadow=local to warnings and fix reported issues (#25316). Feb 13, 2019
@marxin marxin force-pushed the fix-25316-wshadow-local branch 3 times, most recently from a249cc5 to 53e389f Compare February 13, 2019 14:12
@akien-mga akien-mga added this to the 3.1 milestone Feb 13, 2019
SConstruct Outdated Show resolved Hide resolved
@starry-abyss
Copy link
Contributor

I think it makes more sense for names to contain some information about the variable meaning, not just add "2" suffix.

@marxin
Copy link
Contributor Author

marxin commented Feb 14, 2019

I think it makes more sense for names to contain some information about the variable meaning, not just add "2" suffix.

Sure, that makes perfectly sense, but my project knowledge is very low. The "2" suffix was the easiest way how to deal with it. Note that even using loop names as i, j, k can be theoretically improved to a more descriptive name.

@akien-mga
Copy link
Member

I can have a look at each occurrence to find better names if you want, and amend the commit accordingly.

@marxin
Copy link
Contributor Author

marxin commented Feb 14, 2019

I can have a look at each occurrence to find better names if you want, and amend the commit accordingly.

Great, I would appreciate that.

@akien-mga
Copy link
Member

akien-mga commented Feb 20, 2019

I can have a look at each occurrence to find better names if you want, and amend the commit accordingly.

I reviewed a few of occurrences, but eventually had to give up in front of the sheer amount.
I reviewed all of core at least, and replaced the E2 occurrences by F where appropriate, as it's what we do usually.
I also had to fix -Werror=shadow-local in the mono module (cc @neikeq).

return MARSHALLED_IN(Plane, (GDMonoMarshal::M_Plane *)mono_object_unbox(p_obj));

if (mono_class_is_enum(tclass->get_mono_ptr()))
if (mono_class_is_enum(vtclass->get_mono_ptr()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are all the previous tclass locals being renamed?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, GH didn't show me your comment before I clicked "merge" somehow.

In this method, all the tclass in this local scope were shadowing the definition at line 580:

GDMonoClass *tclass = GDMono::get_singleton()->get_class(mono_object_get_class(p_obj));

The renamings in the other methods are actually a mistake, I acted based on a list of -Werror=shadow-local errors and didn't properly check the line numbers.

@akien-mga akien-mga merged commit 8107fc9 into godotengine:master Feb 20, 2019
@akien-mga
Copy link
Member

Thanks @marxin for the impressive work!

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