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 shadow variables #381

Merged
merged 11 commits into from
Jan 19, 2024
Merged

Fix shadow variables #381

merged 11 commits into from
Jan 19, 2024

Conversation

JohanBertrand
Copy link
Contributor

@JohanBertrand JohanBertrand commented Jan 13, 2024

More details in nasa/fprime#2482

Fixes in the code generation to remove -Wshadow warnings:

  • Added m_ for fprime component member variables

@JohanBertrand JohanBertrand marked this pull request as draft January 13, 2024 21:39
@JohanBertrand JohanBertrand marked this pull request as ready for review January 17, 2024 09:41
Copy link
Collaborator

@LeStarch LeStarch left a comment

Choose a reason for hiding this comment

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

I see what I expect....but it seems like certain models where updated. I like those changes, but will defer to @bocchino to determine if they are desired.

@bocchino bocchino self-requested a review January 19, 2024 01:51
We don't want the m_ prefix in the member names. Those
should correspond to the FPP member names. m_ is just for
the C++ implementation.
Copy link
Collaborator

@bocchino bocchino left a comment

Choose a reason for hiding this comment

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

Looks good! I reverted two changes: (1) I restored the old behavior of the toString conversion. The names of the logical struct variables are the names shown in FPP. The m_ names are only for the C++ implementation. (2) I restored the FPP variable names in the unit tests.

@JohanBertrand
Copy link
Contributor Author

Thanks!

I originally changed the names of the variables in the fpp files, because I thought we should not advise users to add a m prefix in front of the variables they defined.

@bocchino bocchino merged commit 5bd815b into nasa:main Jan 19, 2024
11 checks passed
@bocchino
Copy link
Collaborator

bocchino commented Jan 19, 2024

I think it's OK in the context of a unit test. In real code, a struct member name should say what the member does. In a unit test context, it seems fine to call a U32 member mU32, since it really has no purpose except to be a U32 member in the context of that test.

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