-
Notifications
You must be signed in to change notification settings - Fork 50
Cleanup main model: Make components to be forward declared #1142
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Nitish Bharambe <[email protected]>
|
I like the cleanup. Now I wonder, should we move In addition, can we also include |
Signed-off-by: Nitish Bharambe <[email protected]>
Signed-off-by: Nitish Bharambe <[email protected]>
Signed-off-by: Nitish Bharambe <[email protected]>
Signed-off-by: Nitish Bharambe <[email protected]>
Signed-off-by: Nitish Bharambe <[email protected]>
|
Signed-off-by: Nitish Bharambe <[email protected]>
Signed-off-by: Nitish Bharambe <[email protected]>
Signed-off-by: Nitish Bharambe <[email protected]>
Signed-off-by: Nitish Bharambe <[email protected]>
|
figueroa1395
left a comment
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.
One additional thing is that perhaps #1142 (comment) is still relevant. Also, maybe triggering Copilot for review might be useful since missing things for a human due to the nature of this PR is very easy.
Rest looks good, just minor questions.
|
|
||
| WarningsAsErrors: '*' | ||
|
|
||
| IgnoreHeaders: 'Eigen/.*' |
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.
I'm curious. Why was this necessary? If I understand correctly, this prevents clang-tidy from inserting/removing headers in our Eigen dependency?
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.
We would not want eigen to check includes missing within the Eigen library. It is an option only for misc-include-cleaner.
https://clang.llvm.org/extra/clang-tidy/checks/misc/include-cleaner.html
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.
but what you're doing now is a global configuration, not a configuration for misc-include-cleaner only
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.
The option name indeed sounded very generic.
But this option is placed under the include cleaner as per https://clang.llvm.org/extra/clang-tidy/checks/misc/include-cleaner.html.
I think it applies to only the include-cleaner because:
- Check for eg. https://clang.llvm.org/extra/clang-tidy/checks/misc/header-include-cycle.html. There is an ignored files mentioned there. Obviously these options might collide with each other.
- Check this search: https://clang.llvm.org/extra/search.html?q=IgnoreHeaders. The option gets mentioned only in the include-cleaner
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.
The correct syntax for only configuring for a single check is
CheckOptions:
- key: misc-include-cleaner.IgnoreHeaders
value: 'Eigen/.*'
Configuring globally is bad practice and even dangerous in the future



Idea is to make components forward declared to reduce coupling.
This draft PR is created for gathering thoughts and requirements as of now.
Changes proposed in this PR include: