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 -Werror -Wshadow compiler flags to CMAKE_CXX_FLAGS (#531) #535

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vaezim
Copy link

@vaezim vaezim commented Dec 8, 2024

No description provided.

@rh101
Copy link
Contributor

rh101 commented Dec 8, 2024

@vaezim It looks like you accidentally changed code completely unrelated to the -Werror -Wshadow compiler flags. There shouldn't be underscores at the start of all those variable names.

@vaezim
Copy link
Author

vaezim commented Dec 8, 2024

@vaezim It looks like you accidentally changed code completely unrelated to the -Werror -Wshadow compiler flags. There shouldn't be underscores at the start of all those variable names.

Those variables were shadowed by another variable in the outer scopes. Either outer scope variable or local ones needed renaming. Do you have other solutions? we can clear the outer one and reuse it.

@rh101
Copy link
Contributor

rh101 commented Dec 8, 2024

Those variables were shadowed by another variable in the outer scopes. Either outer scope variable or local ones needed renaming. Do you have other solutions? we can clear the outer one and reuse it.

If that's the case, then it would be best to rename them to something more appropriate, without the underscore prefix, since by convention that makes them appear as class member variables, which they are not.

If it is possible to re-use the outer-scope variables, then that is also a solution, but all code paths should be considered to ensure it is safe to re-use them.

@vaezim
Copy link
Author

vaezim commented Dec 8, 2024

Removed underscores and renamed according to scope context.

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.

2 participants