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

kernel: Avoid including files outside include guards #4312

Merged
merged 1 commit into from
Apr 8, 2024

Conversation

jix
Copy link
Member

@jix jix commented Apr 2, 2024

This adjusts the way the headers kernel/{yosys,rtlil,register,log}.h include each other to avoid the need of including headers outside of include guards as well as avoiding the inclusion of rtlil.h in the middle of yosys.h with rtlil.h depending on the prefix of yosys.h, and the suffix of yosys.h depending on rtlil.h.

To do this I moved some of the declaration in yosys.h into a new header yosys_common.h. I'm not sure if that is strictly necessary.

Including any of these files still results in the declarations of all these headers being included, so this shouldn't be a breaking change for any passes or external plugins.

My main motivation for this is that ccls's (clang based language server) include guard handling gets confused by the previous way the includes were done. It often ends up treating the include guard as a generic disabled preprocessor conditional, breaking navigation and highlighting for the core RTLIL data structures.

Additionally I think avoiding cyclic includes in the middle of header files that depend on includes being outside of include guards will also be less confusing for developers reading the code, not only for tools like ccls.

@jix jix requested a review from nakengelhardt April 2, 2024 14:44
This adjusts the way the headers kernel/{yosys,rtlil,register,log}.h
include each other to avoid the need of including headers outside of
include guards as well as avoiding the inclusion of rtlil.h in the
middle of yosys.h with rtlil.h depending on the prefix of yosys.h, and
the suffix of yosys.h depending on rtlil.h.

To do this I moved some of the declaration in yosys.h into a new header
yosys_common.h. I'm not sure if that is strictly necessary.

Including any of these files still results in the declarations of all
these headers being included, so this shouldn't be a breaking change for
any passes or external plugins.

My main motivation for this is that ccls's (clang based language server)
include guard handling gets confused by the previous way the includes
were done. It often ends up treating the include guard as a generic
disabled preprocessor conditional, breaking navigation and highlighting
for the core RTLIL data structures.

Additionally I think avoiding cyclic includes in the middle of header
files that depend on includes being outside of include guards will also
be less confusing for developers reading the code, not only for tools
like ccls.
@jix jix force-pushed the break-cyclic-includes branch from 77f7cca to d8687e8 Compare April 2, 2024 14:54
@jix jix added merge-after-jf Merge: PR will be merged after the next Dev JF unless concerns are raised and removed merge-after-jf Merge: PR will be merged after the next Dev JF unless concerns are raised labels Apr 8, 2024
Copy link
Member

@nakengelhardt nakengelhardt left a comment

Choose a reason for hiding this comment

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

Confirming that all the linty plugins still compile after this change, the earlier problem was just my own local misconfiguration.

@jix jix merged commit eb6c939 into YosysHQ:main Apr 8, 2024
18 checks passed
@jix jix deleted the break-cyclic-includes branch April 8, 2024 18:49
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.

2 participants