Skip to content

Add forward declarations in some headers#1460

Closed
Explorer09 wants to merge 1 commit intohtop-dev:mainfrom
Explorer09:forward-declare
Closed

Add forward declarations in some headers#1460
Explorer09 wants to merge 1 commit intohtop-dev:mainfrom
Explorer09:forward-declare

Conversation

@Explorer09
Copy link
Copy Markdown
Contributor

@Explorer09 Explorer09 commented Apr 19, 2024

I'm not sure this code quality issue is worth fixing in htop, but I hope getting some discussions.

In #1454 I mentioned a "header dependency hell" that motivated me to split the MeterModeId type into a separate header. It is this one.

When I added an #include "Meter.h" line into Settings.h, a circular inclusion of headers would happen and that results in build errors in many .c files.

Meter.h includes Machine.h which includes Settings.h which would then include Meter.h, but due to the header guard, the latter Meter.h include line resolves to no-op. Sometimes a module might include just Machine.h or just Settings.h but the circular inclusion can happen either case.

When a C header depends on a typedef of another type, it has to include the header defining that type as the type redefinition (typedef defining the same type twice) is an error in C. Forward declarations only work with struct, union, enum and class (C++) names, but not typedefs, but htops uses typedefs extensively even for structs in the coding style.

When a structure like Meter only include a reference of another class such as Machine, there's no need to fully define the Machine type nor include the Machine.h in order to use it (in the header; the implementation part, i.e. Meter.c would have to include Machine.h of course). A forward declaration could work equally well for a header.

The commit I presented is not a complete fix. It's a part of header changes that, when using forward declarations, can fix build errors I encountered. Is it a good idea to apply such forward declarations whenever possible in htop headers?

@BenBE BenBE added code quality ♻️ Code quality enhancement needs-discussion 🤔 Changes need to be discussed and require consent labels Apr 19, 2024
@BenBE
Copy link
Copy Markdown
Member

BenBE commented Apr 19, 2024

IMO we should keep forward declarations to a minimum. As htop already uses IWYU for minimizing includes, it's mostly a matter of keeping those recursive dependencies at bay. Apart from some core types, there should – in the best case – be no further such loops.

Regarding this concrete PR: Not a fan of it at all … Personally I find that repeated struct Foo_ instead of just Foo distracting and thus causing the code to be harder to read. Thus those long-form declarations should be kept at a bare minimum, where there's no real other chance to avoiding them.

Also, the code style (although IIRC not explicitly written) encourages to scope includes of headers to be as minimal as possible. If you need a type only for an implementation, only include it with the .c file, instead of making it part of the public interface. Usually IWYU will hint you at this whenever it is possible directly.

@Explorer09
Copy link
Copy Markdown
Contributor Author

Is there anyone else wanting to comment on this issue? I am considering closing this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code quality ♻️ Code quality enhancement needs-discussion 🤔 Changes need to be discussed and require consent

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants