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

Add -fstrict-headers #235

Closed
wants to merge 3 commits into from
Closed

Add -fstrict-headers #235

wants to merge 3 commits into from

Conversation

Vexu
Copy link
Owner

@Vexu Vexu commented Feb 5, 2022

Closes #234

Should this be on by default? Should it be an error or a warning? Maybe it should be a warning if it wasn't explicitly set on.

@ghost
Copy link

ghost commented Feb 5, 2022

Great work, but just one tiny nit: indirect dependencies aren't the only way headers can be abused. I've proposed a new name (-Windirect-include) in the original issue, and also proposed another warning (-Wunused-include) for a different header abuse (sorry I'm slow 😞). Perhaps -Wstrict-headers could include both? (NOPE NOPE NOPE NOPE NOPE)

@Vexu
Copy link
Owner Author

Vexu commented Feb 5, 2022

I added the unused include error too but it seems like it might cause a lot of false positives. Could you test it out and see how it feels?

Also should the direct include rule apply to macros too?

@ghost
Copy link

ghost commented Feb 5, 2022

Hooolyyyy mackerel. I just built Janet with the latest commit, and it threw 215 errors, of which only 7 were from the project itself, and maybe one was a true positive. musl isn't quite as bad on the surface, but tedious to trace.

I've changed my mind. -Wunused-include is a bad idea.

(But yes, I do think the direct include rule should apply to macros. I'm thinking of the dev who has to track down all the #defines.)

@Vexu Vexu marked this pull request as ready for review February 5, 2022 18:28
@Vexu Vexu force-pushed the indirect-sym branch 2 times, most recently from 3e39cc7 to 3cf6297 Compare February 7, 2022 13:40
@Vexu
Copy link
Owner Author

Vexu commented Feb 7, 2022

@EleanorNB I re-added the unused import warning with some bug fixes and made it not apply to system headers, wanna give it another try?

@ghost
Copy link

ghost commented Feb 8, 2022

Hm, still throwing the same errors for me, even on branch tip. Most indirect symbol warnings (couldn't count, too many) are for system headers, and some notes suggest using headers from my downloaded copy of Aro to fix them. Same story with unused.

Below is the full error log of compiling https://github.com/janet-lang/janet commit 66e0b53cf655c5981d4e9974d354e3f0240290a8 with branch tip.
https://zigbin.io/881d09

@Vexu
Copy link
Owner Author

Vexu commented Jun 20, 2022

Closing as this never worked in practice and isn't a high priority. Maybe someday someone will finish it.

@Vexu Vexu closed this Jun 20, 2022
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.

Proposal: Warn for use of a symbol not declared in the same file or an explicitly included header
1 participant