Rust: Implement UnusedValue.ql (2) #17773
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The query is indeed quite noisy with spurious results in real world code. I've just added some more test cases that show some of them. However with the current metadata the query isn't part of a suite and so isn't run at all on DCA. So it won't cause any problems to merge what we have now and follow up with improvements (and increase the |
aibaars
approved these changes
Oct 17, 2024
There was a problem hiding this comment.
Looks good to me. Perhaps @paldepind would like to have a look as well?
|
Yep, and maybe hit merge if you're happy (I'll be away for a week). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements an initial version of
UnusedValue.ql, based on SSA, originally written by @hvitved in #17726 . I've already stated I'm happy with the QL changes, but that PR can't be merged while Tom is away due to merge conflicts.I have added some merge fixup (for extra test cases that have been added in
main) and will start a DCA run on this PR as well. Results are generally very good but there are a few spurious ones in the new tests aroundasserts, which I'm worried will translate to a lot of noise on DCA...