boinc icon indicating copy to clipboard operation
boinc copied to clipboard

Remove unused functions

Open Vulpine05 opened this issue 1 year ago • 5 comments

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 avatar Oct 18 '24 02:10 Vulpine05

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.

Vulpine05 avatar Oct 18 '24 03:10 Vulpine05

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.

CharlieFenton avatar Oct 18 '24 08:10 CharlieFenton

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:

https://github.com/BOINC/boinc/blob/be884908dc15de5c3d75739f9174548a18c7fc5f/client/project.h#L390

Vulpine05 avatar Oct 19 '24 01:10 Vulpine05

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

codecov[bot] avatar Oct 20 '24 02:10 codecov[bot]

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

Vulpine05 avatar Oct 20 '24 03:10 Vulpine05

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

AenBleidd avatar Oct 22 '24 13:10 AenBleidd

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

Vulpine05 avatar Oct 22 '24 23:10 Vulpine05