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

Remove unused functions #5850

Merged
merged 7 commits into from
Oct 22, 2024
Merged

Conversation

Vulpine05
Copy link
Contributor

Description of the Change
Browsing through the code, I have found several functions defined in header files that Visual Studio found did not have an associated function nor inline code. Since these functions do not exist, this PR removes them.

I have broken up the PR into three commits; each commit is for a subfolder (client, manager, and lib).

This PR does not clean up ALL unused functions. There were some functions that were empty constructors that I didn't feel comfortable removing. Another thing I discovered were some functions were noted by Visual Studio as not defined, but the function was written in one or more preprocessor definitions. With that said, I tried to carefully remove only functions that were obvious to me that weren't defined. Thank you in advance for reviewing!

@Vulpine05
Copy link
Contributor Author

Oops, I don't see how I missed that Start function, but it makes sense now. I'll fix that when I'm able to.

@CharlieFenton
Copy link
Contributor

Please do not remove the following line from MainDocument.h:
// void TestAsyncRPC(); // For testing Async RPCs
The comment clearly states that this line has been left in the code but disabled with // to allow easily re-enabling it for additional testing when needed. The corresponding function is in AsyncRPC.cpp and disabled by #if 0 and #endif, allowing for easy re-enabling when needed.

Visual Studio found did not have an associated function nor inline code.

Since this line is disabled with //, I don't see how the compiler would mark it as a declaration with no corresponding function.

Also, please use caution when removing declarations, because some functions are used only by builds for certain operations systems (Windows, MacOS, Linux, Android, etc.) Ideally both the header declaration and function definition will be guarded by something like #ifdef _WIN32 or #ifdef __WXMAC__, but there might be instances where that guard is missing from the header declaration. That situation would also make the compiler think there is a header declaration with no associated function. Hopefully, the CI build will fail if you remove a declaration needed when building for a different operating system.

@Vulpine05
Copy link
Contributor Author

Please do not remove the following line from MainDocument.h: // void TestAsyncRPC(); // For testing Async RPCs The comment clearly states that this line has been left in the code but disabled with // to allow easily re-enabling it for additional testing when needed. The corresponding function is in AsyncRPC.cpp and disabled by #if 0 and #endif, allowing for easy re-enabling when needed.

Understood, apologies that I missed that the first time around.

Since this line is disabled with //, I don't see how the compiler would mark it as a declaration with no corresponding function.

It didn't find it that way, I just thought it was commented out for not being used, but I was too quick with the delete key for that one. This was an exception to how I was targeting unused functions. Again, apologies.

Also, please use caution when removing declarations, because some functions are used only by builds for certain operations systems (Windows, MacOS, Linux, Android, etc.) Ideally both the header declaration and function definition will be guarded by something like #ifdef _WIN32 or #ifdef __WXMAC__, but there might be instances where that guard is missing from the header declaration. That situation would also make the compiler think there is a header declaration with no associated function. Hopefully, the CI build will fail if you remove a declaration needed when building for a different operating system.

Yes, I did see some of those, and for the most part I left them alone because I didn't want to break something I didn't fully understand. For all other functions I did painstakingly search through the entire code to see if the function was used. If I didn't find it anywhere else, I removed it. There are some instances where a function was marked as not used, but something else was happening (different input variables, or preprocessor declarations for example), and I left those alone since it wasn't just removing a declaration.

The only declaration I see right now I have removed is part of a preprocessor is here:

void update_dcf_stats(RESULT*);

Copy link

codecov bot commented Oct 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 10.78%. Comparing base (dca4153) to head (aa35989).
Report is 18 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #5850      +/-   ##
============================================
+ Coverage     10.51%   10.78%   +0.26%     
  Complexity     1068     1068              
============================================
  Files           280      280              
  Lines         36021    36448     +427     
  Branches       8441     8441              
============================================
+ Hits           3789     3930     +141     
- Misses        31843    32129     +286     
  Partials        389      389              
Files with missing lines Coverage Δ
lib/app_ipc.h 0.00% <ø> (ø)
lib/common_defs.h 0.00% <ø> (ø)
lib/gui_rpc_client.h 0.00% <ø> (ø)
lib/prefs.h 0.00% <ø> (ø)
lib/str_util.h 0.00% <ø> (ø)

... and 90 files with indirect coverage changes

@Vulpine05
Copy link
Contributor Author

Okay, take two. I think this is ready for review now.

@AenBleidd
Copy link
Member

Ok, it was not easy.
Looks good to me.

@AenBleidd AenBleidd merged commit cdedb8a into BOINC:master Oct 22, 2024
146 checks passed
@Vulpine05 Vulpine05 deleted the Vulpine05_Unused_func branch October 22, 2024 23:34
@Vulpine05
Copy link
Contributor Author

Thank you for reviewing, @AenBleidd. I'm sure it was harder than it seemed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

3 participants