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

Clean up some code duplication in the header files #123

Merged
merged 4 commits into from
Sep 14, 2020

Conversation

hugomg
Copy link
Contributor

@hugomg hugomg commented Sep 12, 2020

PR #70 got rid of the infrastructure for generating header files, but it left behind some code duplication.

Some of cases are things that belong in the header file and don't need to be repeated in the C file. Other cases are things that belong in the C file and don't need to be in the header file.

In this commit I tried to fix all of these that I could find. When given a choice I preferred keeping things out of the header file, unless they were being used by someone else.

PR htop-dev#70 got rid of the infrastructure for generating header
files, but it left behind some code duplication.

Some of cases are things that belong in the header file and don't need
to be repeated in the C file. Other cases are things that belong in the
C file and don't need to be in the header file.

In this commit I tried to fix all of these that I could find. When given
a choice I preferred keeping things out of the header file, unless they
were being used by someone else.
Copy link
Member

@BenBE BenBE left a comment

Choose a reason for hiding this comment

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

Maybe also have a look if we could utilize IWYU

@hugomg
Copy link
Contributor Author

hugomg commented Sep 13, 2020

Using an static analysis tool to further clean up the headers sounds like an interesting idea but maybe it would be more appropriate for a different pull request? For this one I just used a script to identify lines of code that were repeated in both the .c and .h file.

As suggested by cppclean.
@hugomg
Copy link
Contributor Author

hugomg commented Sep 13, 2020

I ran the code through cppclean. There were only a handful of includes that were detected as truly unused so I went ahead and removed them.

Cppclean also pointed out several situations where we are only including a header file to be able to reference a pointer type from the header file. In theory those includes could be replaced by a forward declaration but I'm not sure if it would be worth the hassle. At least not for this PR, I guess.

./Action.h:10: 'Header.h' does not need to be #included
./Action.h:12: 'ProcessList.h' does not need to be #included
./Action.h:14: 'UsersTable.h' does not need to be #included
./Header.h:12: 'Vector.h' does not need to be #included
./MainPanel.h:12: 'IncSet.h' does not need to be #included
./Object.h:12: 'XAlloc.h' does not need to be #included
./Object.h:13: 'Macros.h' does not need to be #included
./ProcessList.h:10: 'Vector.h' does not need to be #included
./ProcessList.h:15: 'Settings.h' does not need to be #included
./RichString.h:11: 'config.h' does not need to be #included
./ScreenManager.h:10: 'Vector.h' does not need to be #included
./Settings.h:12: 'Process.h' does not need to be #included
./darwin/DarwinProcessList.h:22: 'zfs/ZfsArcStats.h' does not need to be #included
./darwin/Platform.h:13: 'CPUMeter.h' does not need to be #included
./darwin/Platform.h:14: 'BatteryMeter.h' does not need to be #included
./darwin/Platform.h:15: 'DarwinProcess.h' does not need to be #included
./dragonflybsd/DragonFlyBSDProcessList.h:20: 'DragonFlyBSDProcess.h' does not need to be #included
./dragonflybsd/Platform.h:12: 'BatteryMeter.h' does not need to be #included
./freebsd/FreeBSDProcessList.h:10: 'zfs/ZfsArcStats.h' does not need to be #included
./freebsd/Platform.h:11: 'BatteryMeter.h' does not need to be #included
./linux/IOPriorityPanel.h:12: 'ListItem.h' does not need to be #included
./linux/LinuxProcessList.h:11: 'zfs/ZfsArcStats.h' does not need to be #included
./linux/Platform.h:11: 'MainPanel.h' does not need to be #included
./linux/Platform.h:12: 'BatteryMeter.h' does not need to be #included
./linux/Platform.h:13: 'LinuxProcess.h' does not need to be #included
./openbsd/Platform.h:12: 'BatteryMeter.h' does not need to be #included
./solaris/Platform.h:13: 'BatteryMeter.h' does not need to be #included
./solaris/SolarisProcessList.h:16: 'zfs/ZfsArcStats.h' does not need to be #included
./unsupported/Platform.h:12: 'BatteryMeter.h' does not need to be #included
./unsupported/Platform.h:14: 'UnsupportedProcess.h' does not need to be #included

@BenBE
Copy link
Member

BenBE commented Sep 13, 2020

@hugomg Only suggested IWYU for future work, as building with it can sometimes be a bit of a hassle. I'll open another issue for consideration and further investigation.

Regarding pointer to structs: I usually don't really like having bare forward declarations floating around, unless the headers that would have to be included otherwise would be some massive overhead. As things are now you'd probably won't even notice the additional disk IO, even when trying to compile with your storage on the far side of a slow network …

@hugomg
Copy link
Contributor Author

hugomg commented Sep 13, 2020

Oh... of course.

My impression with respect to the header files is that the compilation time for each file is short enough that shaving off some includes doesn't make a big difference. I suspect that the more impactful change would be things that reduce the number of C files that need to be recompiled when a header file changes. For example, refactoring a class to remove the definition of the struct from the header file, making the fields private. I'm not sure if it would be worth the trouble but it would be something to keep in mind.

@BenBE
Copy link
Member

BenBE commented Sep 13, 2020

IMHO compilation is fast enough as is. No need to put much work in there.

@natoscott natoscott merged commit ccf0960 into htop-dev:master Sep 14, 2020
@hugomg hugomg deleted the header-duplicates branch September 14, 2020 16:29
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