-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Force include lib/ files with full path #9387
Conversation
I use following command to convert includes: for i in support core asn1 mdns shell; do git sed "#include \"$i/\\(.*\\)\"\$" "#include <lib/$i/\\1>" ; done
for i in support core asn1 mdns shell; do git sed "#include <$i/\\(.*\\)>\$" "#include <lib/$i/\\1>" ; done |
Note, @kghost - this has real build failures. |
Please:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So comparing this to the output of running the commands claimed and doing the fixups that are needed to fix the build:
- Why the blank line additions to
src/app/zap-templates/templates/app/af-structs.zapt
? - Why the removal of the
ASN1OID.h
include fromsrc/credentials/tests/CHIPCert_test_vectors.h
? - Why the removal of the
ASN1OID.h
include fromsrc/lib/asn1/tests/TestASN1.cpp
? - Why the removal of the
ASN1OID.h
include fromsrc/tools/chip-cert/chip-cert.h
?
I'm already unable to trace manual fixes in the PR. It is around ten or twenty manual fixes. I can make sure that only includes are changes, and their order due to restyle. |
Prevent restyle from changing their order. Although the generate file is not processed by restyle, but we should follow the code style.
Above includes doesn't put the file in the namespace, so causing compile error, it is probably a bug hidden behind the include order. |
There are manual fixes in 30 files or so, most of them in just a few categories. For future, to make this sort of thing reviewable, I would recomment one changeset with the script-generated mass-change, documented with what the scripts involved were (so reviewers can easily check that nothing else is in that changeset; in this case that scripted mass-change would be the sed scripts, restyled-io, and ZAP codegen) and one changeset with the manual changes that actually need to be manually reviewed....
Right, given the scripts I manually verified things; that's where #9387 (review) comes from. :) |
Oh, wow. That is ... not sane. Please file a followup issue to fix that and just put the right namespacing info in the generated file instead of doing that? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved on condition that the commit message is updated as I requested:
- The scripts used
- The git-extras package needed
- General summary of manual changes (undo the changes for the SDK shell stuff, modify gn files to actually do the forcing of full path, stop including the IOD header outside of the namespace bits, that seems like about it?)
And thank you for doing this! |
I'll split them into 2 patches, but the script generated patch will not compile, so it is unable to separate them into 2 PRs
Not problem |
b2aec2f
to
1957463
Compare
The reason this didn't happen in the last round of cleanups is that lib/ itself is inconsistent. What criteria defines whether something goes in lib/ or not? I can't think of one. So there are two options
|
Or option 3) move more code into lib/ in some principled way But ideally our includes would actually look like this #include <matter/...> for which we might consider A) Rename src/ to matter/ (this is orthogonal to whether lib/ is kept or not) |
This goal of this PR is to disambiguate the include path of It is fine to re-arrange files, and do cleanups, in the following up PRs, but they have nothing to do with this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approve assuming CI pases and changes are only include changes. I am unable to properly review because Github UI uses 100% CPU and fails to easily show me the deltas.
@kghost - the mbedos failure may be real. |
This is auto generated by using following commands: for i in support core asn1 mdns shell; do git sed "#include \"$i/\\(.*\\)\"\$" "#include <lib/$i/\\1>" ; done for i in support core asn1 mdns shell; do git sed "#include <$i/\\(.*\\)>\$" "#include <lib/$i/\\1>" ; done Note: git-sed is in the git-extras package
8e77155
to
da0805f
Compare
Size increase report for "esp32-example-build" from a8f02e2
Full report output
|
You can download the patch.. |
Indeed. And given that @kghost split this up into multiple changesets, can verify that the script-generated changeset matches the script output and review the small manual bits.... That's what I ended up doing. |
Problem
There are two ways to include files under
src/lib
,#include <lib/core/xxx.h>
or#include <core/xxx.h>
. It turns out that mixing these two include may confuse#pragma once
and causing redefinition error.Change overview
Disallow using #include <core/xxx.h>. force use the full path with 'lib/'.
Testing
Verified using unit-tests