Fix shadow variables #2482

JohanBertrand · 2024-01-13T02:10:26Z


*Related Issue(s)*	None
*Has Unit Tests (y/n)*	N/A
*Documentation Included (y/n)*	N/A

Change Description

Add support of -wShadow in the code base.

Forseen changes are:

Add a prefix m_ to private and protected class members
Add a prefix a_ to arguments shadowing a public class member
Change variable names to avoid shadowing class member
Remove redundant variable definition with same name

The a_ prefix has been chosen to avoid breaking changes in the code. Changing the name of public variables can have a bigger impact on other projects. However, I think some of those variables should become protected instead of public, and the _m prefix should be used on those class members in the future. I think it's an open point of discussion.

A full compatibility with -wShadow will require some changes also in fpp. See nasa/fpp#381
I am not sure what would be the release schedule. We might want to keep the -Wshadow flag out until the next fpp release happens.

Rationale

Shadow variables can make the code more difficult to read and debug.

Testing/Review Recommendations

TBD

LeStarch

I think there is only one required change I see: comment out the -Wshadow in the
Ref package until the PR on FPP makes its way over.

If you are so-motivated, there are a few other minor comments on other cleanup items....however, none of these are required in this PR. I'll take the action to add tickets as we need something to track this work.

@bocchino or @thomas-bc this is a big PR but entirely formulaic, would one of you take a look to ensure I did not miss anything?

CMakeLists.txt

Fw/FilePacket/DataPacket.cpp

Ref/CMakeLists.txt

LeStarch · 2024-01-18T17:08:14Z

STest/STest/Rule/Rule.hpp

@@ -27,9 +27,9 @@ namespace STest {

      //! Construct object Rule
      Rule(
-          const char *const name //!< The name of the rule
+          const char *const a_name //!< The name of the rule


@bocchino we will need to look into this w.r.t. the STest repository. If and when we move to a full sub-repo, we'll need to ensure shadows are weeded-out.

I'll leave that to you to track before we make STest a subrepo.

LeStarch · 2024-01-18T17:14:23Z

Svc/FileUplink/FileUplink.hpp

          }

        PRIVATE:

          //! The total number of warnings
-          U32 n;


Another minor nit, not blocking for this PR....but given this PR is changing much, could we rename this to m_warning_count or something more descriptive?

I changed the m_n variable names to something more descriptive, and updated the comments of the variables. Let me know if there is a mistake in the comments

Fw/FilePacket/FilePacket.hpp

@@ -291,7 +291,7 @@
      // Constructor
      // ----------------------------------------------------------------------

-      FilePacket() { this->header.type = T_NONE; }
+      FilePacket() { this->m_header.type = T_NONE; }


Svc/FileDownlink/FileDownlink.hpp

@@ -43,32 +43,32 @@
        public:

          //! Constructor
-          Mode() : value(IDLE) { }
+          Mode() : m_value(IDLE) { }


github-advanced-security

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

LeStarch · 2024-01-18T17:46:36Z

The CI failures are likely due to unit testing. Much of our unit testing relies on white-box testing and as such does access these private members. We'll need to fix those too. @JohanBertrand are you up for this fix?

LeStarch · 2024-01-18T17:48:09Z

In case this isn't clear, this is solid work. Thanks for the contribution. Feel free to communicate us if you have reached the limit of what you are able to work on and we can move it through the process, otherwise we'll let you move it forward!

JohanBertrand · 2024-01-18T18:19:23Z

I think there is only one required change I see: comment out the -Wshadow in the Ref package until the PR on FPP makes its way over.

From what I've seen, it works well because the warning flags apply only to the user code, and not to the auto generated code. The unit tests and the build on Ref were working fine for me.

If you are so-motivated, there are a few other minor comments on other cleanup items....however, none of these are required in this PR. I'll take the action to add tickets as we need something to track this work.

I'm happy keeping the work going, all good. I would definitely do the fix of the a_ prefix in another PR to make it easier to review, but I'll go through the rest.

The CI failures are likely due to unit testing. Much of our unit testing relies on white-box testing and as such does access these private members. We'll need to fix those too. @JohanBertrand are you up for this fix?

It was compiling well for me. I already did changes in the test files, but just a few were needed. I did not run the check though. I'll double check, thanks.

Does this relate to the failure in CI / Framework (pull_request)?

In case this isn't clear, this is solid work. Thanks for the contribution. Feel free to communicate us if you have reached the limit of what you are able to work on and we can move it through the process, otherwise we'll let you move it forward!

Thanks for the kind words, I'll come back with the changes in a few hours/days.

JohanBertrand · 2024-01-21T21:15:51Z

@LeStarch it should be good to go, let me know if something else is need

I'll work on #2492 once this PR is closed

LeStarch · 2024-01-23T19:25:41Z

I fixed a comment. We'll set what CI does, if it fails I can render more advice!

JohanBertrand · 2024-01-26T21:19:06Z

Is there anything else needed before merging?

LeStarch · 2024-02-01T17:58:08Z

I have approved, was hoping for one more set of eyes on this because of the extensive length of the PR. Once we get that it will be merged!

JohanBertrand added 2 commits January 13, 2024 02:54

Fixed shadow warnings from Fprime

5a0cea9

Fix unit tests

63eae9a

JohanBertrand marked this pull request as draft January 13, 2024 02:10

JohanBertrand changed the title ~~draft: Fix shadow variables~~ Fix shadow variables Jan 13, 2024

JohanBertrand added 3 commits January 13, 2024 21:26

Fix missing shadow warnings

4f95081

Fix condition in cmake

c0019f1

Fix cmake

e400373

JohanBertrand mentioned this pull request Jan 15, 2024

Fix shadow variables nasa/fpp#381

Merged

JohanBertrand marked this pull request as ready for review January 18, 2024 16:00

LeStarch requested changes Jan 18, 2024

View reviewed changes

github-advanced-security bot found potential problems Jan 18, 2024

View reviewed changes

LeStarch linked an issue Jan 18, 2024 that may be closed by this pull request

Resolve Warnings for -Wshadow #2491

Closed

LeStarch mentioned this pull request Jan 18, 2024

Resolve Shadowed public Members #2492

Closed

LeStarch requested review from thomas-bc and bocchino January 18, 2024 17:49

JohanBertrand added 2 commits January 21, 2024 21:22

Fixes from review

f336af3

Fixed mistake in PathName

d44aea8

Fixing comment

7e0c446

LeStarch approved these changes Feb 1, 2024

View reviewed changes

thomas-bc approved these changes Feb 1, 2024

View reviewed changes

LeStarch merged commit f0f19ba into nasa:devel Feb 1, 2024
34 checks passed

JohanBertrand mentioned this pull request Feb 1, 2024

Removed a_ prefix on arguments #2513

Merged

LeStarch added the Update Instructions Needed Need to add instructions in the release notes for updates. label Feb 14, 2024

JohanBertrand deleted the Fix-shadow-variables branch April 29, 2024 22:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix shadow variables #2482

Fix shadow variables #2482

JohanBertrand commented Jan 13, 2024 •

edited

Loading

LeStarch left a comment

LeStarch Jan 18, 2024

LeStarch Jan 18, 2024

JohanBertrand Jan 21, 2024

github-advanced-security bot left a comment

LeStarch commented Jan 18, 2024

LeStarch commented Jan 18, 2024

JohanBertrand commented Jan 18, 2024

JohanBertrand commented Jan 21, 2024

LeStarch commented Jan 23, 2024

JohanBertrand commented Jan 26, 2024

LeStarch commented Feb 1, 2024

Fix shadow variables #2482

Fix shadow variables #2482

Conversation

JohanBertrand commented Jan 13, 2024 • edited Loading

Change Description

Rationale

Testing/Review Recommendations

LeStarch left a comment

Choose a reason for hiding this comment

LeStarch Jan 18, 2024

Choose a reason for hiding this comment

LeStarch Jan 18, 2024

Choose a reason for hiding this comment

JohanBertrand Jan 21, 2024

Choose a reason for hiding this comment

github-advanced-security bot left a comment

Choose a reason for hiding this comment

LeStarch commented Jan 18, 2024

LeStarch commented Jan 18, 2024

JohanBertrand commented Jan 18, 2024

JohanBertrand commented Jan 21, 2024

LeStarch commented Jan 23, 2024

JohanBertrand commented Jan 26, 2024

LeStarch commented Feb 1, 2024

JohanBertrand commented Jan 13, 2024 •

edited

Loading