Skip to content

Make 3c -verify an error for now. #519

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

Merged
merged 2 commits into from
Mar 30, 2021
Merged

Conversation

mattmccutchen-cci
Copy link
Member

@mattmccutchen-cci mattmccutchen-cci commented Mar 30, 2021

PR #488 made 3c -verify cover only the compiler diagnostics, but none of our regression tests actually use that functionality. Instead, one regression test (macro_function_call) used 3c -verify to try to test the absence of 3C warnings, and we were unaware that the test wasn't testing what it was supposed to. I think it's best to make 3c -verify an error for now so we don't make that mistake again.

PR #488 made `3c -verify` cover only the compiler diagnostics, but none
of our regression tests actually use that functionality. Instead, one
regression test (macro_function_call) used `3c -verify` to try to test
the absence of 3C warnings, and we were unaware that the test wasn't
testing what it was supposed to. I think it's best to make `3c -verify`
an error for now so we don't make that mistake again.

I'm deleting the addVerifyAdjuster code because I think we're unlikely
to ever want to use it. If we do, it should be easy enough to bring back
by reverting this commit.
@kyleheadley
Copy link
Member

I'd prefer you leave the unused code with a "TODO: #425", until we give up on it entirely and close that issue. I'm still hopeful that we can try again and get it to work, since it's such a great tool. Your error message should prevent it from causing problems, and the issue can be amended to include a reminder to delete TODOs on close.

@mattmccutchen-cci
Copy link
Member Author

I'd prefer you leave the unused code with a "TODO: #425", until we give up on it entirely and close that issue. I'm still hopeful that we can try again and get it to work, since it's such a great tool.

Sure; done. I don't feel strongly about this. I was assuming that if we did get the diagnostic verifier to work for 3C diagnostics (as opposed to ordinary compiler diagnostics), we would need different code, but I may be wrong; I haven't researched it.

Copy link
Member

@kyleheadley kyleheadley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@mattmccutchen-cci mattmccutchen-cci merged commit 032a930 into main Mar 30, 2021
@mattmccutchen-cci mattmccutchen-cci deleted the verify-unsupported branch March 30, 2021 20:03
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.

2 participants