include: use angle brackets for exported headers, and check it - #4375
include: use angle brackets for exported headers, and check it#4375grandixximo wants to merge 2 commits into
Conversation
|
Since I cleaned the tree we can enforce it and error the CI when mistakes are done, objections? Anyway Bertho won't let them pass ;-) |
The build copies every SRCHEADERS entry into include/, so an exported header exists twice: the source under src/ and the copy modules compile against. A quoted include searches the includer's own directory before the -I path, an angled include does not, so the two forms can reach different copies of the same header and both compile. An exported header travels to include/ and takes its siblings with it, so it keeps the quoted form and finds them wherever it lands, and so does its implementation, wanting the source rather than a stale export. The files changed here are users, and build out of tree where only the exported copies exist.
Nothing catches the wrong form today, since both compile, and the quoted one silently resolves to whichever copy sits nearest. The check reads the SRCHEADERS list, so it follows whatever the build exports. Exported headers are left alone: they are copied to include/ and have to keep finding their siblings there. A header whose own directory also holds code that merely uses it needs its implementation named, inifile.hh and hal.h being those cases in tree today. Everywhere else being in the header's directory is enough. Findings are warnings by default and errors with --error, which is how CI runs it, and named files can be passed for use from a pre-commit hook.
004a0ad to
b3e6a46
Compare
|
BTW, I don't think we should enforce this in CI. At least not yet. I'd like to see more cleanup before we start enforcing. The cleanup may expose situations that require a second thought. |
|
Fair, I'll keep them as warning for now then. |
| INTERFACES = { | ||
| "src/emc/ini/inifile.hh": ( | ||
| "src/emc/ini/inifile.cc", | ||
| ), | ||
| "src/hal/hal.h": ( | ||
| "src/hal/hal_lib.c", | ||
| "src/hal/hal_lib_extra.c", | ||
| "src/hal/hal_lib_query.c", | ||
| ), | ||
| } | ||
|
|
There was a problem hiding this comment.
The cases here are a requirement that they use the local version. Building the library code is local.
The problem with this is that there are many exceptions and marginal calls with the current organization.
Try emc or motion. The attempt to extract some of the constants was only partly done. All of that needs a cleanup too.
That I need to rephrase. I don't think it should be checked in CI yet, at all. (see my added review comment) The code organization is still messy. That is something that needs fixing first. We need to have (all) the libraries in their own isolated directory and analyze the cross-dependencies. |
|
Ok, I see, I'll get on the cleaning first then, but I'm taking a break tomorrow, traveling with family, I'll be around a bit less... |
2 participants
This has bitten me many times in review, and it is a rule a script can check, so here is the script plus the tree made consistent with it.
The build copies every SRCHEADERS entry into
include/, so an exported header exists twice: the source undersrc/and the copy modules compile against. A quoted include searches the includer's own directory first, an angled include does not, so the two forms can reach different copies of the same header. Both compile, always, which is why it is easy to get wrong and invisible until somebody reads the diff.The implementation keeps quotes, wanting the source beside it rather than a stale export. Everything else is a user, builds out of tree where only the exported copy exists, and takes angle brackets.
20 includes change, no code.
scripts/include-style-check.pyreads the SRCHEADERS list, so it follows what the build exports rather than a list of its own. Findings are warnings by default and errors with--error, which is how the cppcheck job runs it. Named files can be passed, which suits a pre-commit hook. It annotates the offending lines when it runs in CI, so the next case shows up on the diff before a reviewer has to write it out.One judgment call sits in the script rather than in the rule: a header whose own directory also holds code that only uses it needs its implementation named.
inifile.hhandhal.hare those cases in tree today.Full build clean. The check reports nothing on the result, and reports each of the 20 again when they are put back one at a time.