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

Dealing with headers that use #error #47225

Closed
Dr15Jones opened this issue Jan 31, 2025 · 14 comments
Closed

Dealing with headers that use #error #47225

Dr15Jones opened this issue Jan 31, 2025 · 14 comments

Comments

@Dr15Jones
Copy link
Contributor

When doing code migrations, it is useful to allow headers which are not supposed to be used (or possible not used directly) to use the c pre-processor directive #error. The difficulty is our header file consistency check reports these errors. These errors are intentional so should not cause a failure of the header file consistency (in fact, it would be even better if the header did NOT report an error that it caused a failure in the test).

We should determine a way to keep these cases from causing the header consistency from reporting a problem.

@Dr15Jones
Copy link
Contributor Author

assign core

@cmsbuild
Copy link
Contributor

New categories assigned: core

@Dr15Jones,@makortel,@smuzaffar you have been requested to review this Pull request/Issue and eventually sign? Thanks

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 31, 2025

cms-bot internal usage

@cmsbuild
Copy link
Contributor

A new Issue was created by @Dr15Jones.

@Dr15Jones, @antoniovilela, @makortel, @mandrenguyen, @rappoccio, @sextonkennedy, @smuzaffar can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

@Dr15Jones
Copy link
Contributor Author

@makortel and I had a quick discussion on this issue and came up with two possible ideas

  • When running the header file consistency check, use a particular -D directive. Any header file using #error would use a #ifndef on that directive to NOT issue the error in that case. This is easy to maintain but intrusive to the header and would not help catch a case where the error SHOULD go off but does not.
  • We add another step to the header consistency check that uses a list of headers that are supposed to issue errors. If those headers do issue an error the check does not report anything wrong. If they fail to generate an error the checker does mark that as a failure.

@Dr15Jones
Copy link
Contributor Author

@smuzaffar your thoughts on this would be very appreciated.

@smuzaffar
Copy link
Contributor

@Dr15Jones , instead of keeping a hard coded list of headers to ignore, can we just ignore any header which generates : error: #error message ? I can update header consistency check to ignore such headers .

@Dr15Jones
Copy link
Contributor Author

Seems reasonable to me.

@Dr15Jones
Copy link
Contributor Author

@smuzaffar Do #warning also lead to failure in the header consistency checker?

@smuzaffar
Copy link
Contributor

smuzaffar commented Jan 31, 2025

@smuzaffar Do #warning also lead to failure in the header consistency checker?

No. scram just tries to compile a header and if compiler generates any error then scram treats it as failure. So #warning should not lead to failure

@smuzaffar
Copy link
Contributor

cms-sw/cms-bot#2427 fixes bot code to not report headers which explicitly use #error
IB headers checks are also fixed to ignore such headers and latest header consistency checks are passed for 15.0.X

@smuzaffar
Copy link
Contributor

+core

I guess we can close this issue

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 3, 2025

This issue is fully signed and ready to be closed.

@makortel
Copy link
Contributor

makortel commented Feb 3, 2025

@cmsbuild, please close

@cmsbuild cmsbuild closed this as completed Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants