Skip to content

clang::Diagnostic::dispose should be a Drop impl for clang::Diagnostic #145

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.

Sign up for GitHub

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

Jump to bottom
Closed
fitzgen opened this issue Oct 25, 2016 · 2 comments
Closed

clang::Diagnostic::dispose should be a Drop impl for clang::Diagnostic #145

fitzgen opened this issue Oct 25, 2016 · 2 comments
Labels
C-assigned E-less-easy I-cleanup

Comments

@fitzgen
Copy link
Member

fitzgen commented Oct 25, 2016

dispose destroys the diagnostic message and it is unsafe to use after having called dispose. It should be a Drop implementation instead of a method that even makes it possible to continue to use after it has been destroyed.

I don't know how much we depend on this messed up behavior, so this involves some trial and error and investigation into our uses of Diagnostic. It would be a good second or third bug, so marking this E-less-easy rather than E-easy.
@fitzgen
Copy link
Member Author

fitzgen commented Oct 25, 2016

Also, we don't ever even call dispose right now >.<

@KiChjang
Copy link
Member

Investigating this.

@KiChjang KiChjang mentioned this issue Oct 30, 2016
Merged
bors-servo pushed a commit that referenced this issue Oct 30, 2016
Auto merge of #168 - KiChjang:diagnostic-drop, r=emilio
1460787 
Implement Drop for Diagnostic

I built it and ran the test, and they all pass. Not sure what else there is to test for.

Fixes #145.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-assigned E-less-easy I-cleanup
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@fitzgen @KiChjang