C++: Clean up cpp/non-constant-format
#15875
Conversation
…g something as a return value, and a function updating one of its arguments.
…C. This brings back SAMATE results.
…e same exact sources before and after.
…his fixes performance on ImageMagick.
MathiasVP
force-pushed
the
bring-back-type-barriers-in-non-constant-format
branch
from
17d5db2 to
ab6e2f9
Compare
Correct. |
1. It fixes a logic error in the cannotContainString predicate. 2. It reverts the changes to the `isSource` predicate that required the external function to be within the source root. The change to `isSource` was meant to fix the a performance problem that occurred because of the logic error in the cannotContainString predicate. However, now that the logic error is fixed this is no longer necessary 🎉
|
@geoffw0 sorry about all the back-and-forth on this PR. 61597f5 fixes the logic error you spotted, and makes all the controversial source root changes unnecessary 🎉. I've verified that this PR still fixes the performance problems on ImageMagick even when running on #15599. I'm running a final DCA run now to make sure I didn't introduce any problems. I expect that the DCA run will still report some lost results because of the two latter notes I posted in the PR description. I still think that these are changes we're happy with, though. |
There was a problem hiding this comment.
Changes and DCA LGTM. I'm fairly sure we could iterate to get some of those lost results back, but there actually aren't very many lost results at the moment as a proportion of the total number of results found by the query (and as you said, some of them are duplicates anyway).
| not unspecified instanceof Char32Type and | ||
| // C often defines `wchar_t` as `unsigned short` | ||
| not unspecified instanceof UnsignedShort | ||
| | |
There was a problem hiding this comment.
In nelson, wchar_t appears to be a typedef for int (not even unsigned int). I'm not sure if we want to try and expand this to all integral types though, or cut our losses here?
There was a problem hiding this comment.
Ouch that's strange. Thanks for spotting this. Let's just get this merged now and we can revisit this at some point later if necessary. I'll create an issue for it.
There was a problem hiding this comment.
I'm not really convinced it's worth fixing, unless it turns out to be common.
Indeed, this gets rid of tons of duplication: All the I totally agree that we could iterate more on this, but I also don't think it's worth it. FWIW, the lost results on So I'd say that these are FPs that are now fixed. |
This PR does a bunch of cleanups to the
cpp/non-constant-formatas a follow-up to the changes in #15516. In particular, two things is done:[node.asExpr(), node.asIndirectExpr]everywhere.cannotContainStringbarrier that was (accidentially) removedDllMaincontinue to be sources.With these
twothree changes, we're once again able to run the query onImageMagick/ImageMagickeven with the changes in #15599I've looked at the DCA results, and here are my findings:
std::vector.The last one is a bit of a shame, but it should be properly fixed once we have MaD and can model precise collection flow (I'll make sure to backlink to this PR from the internal issue). And as the MaD work is coming closer to a mergeable state every day, I think we can live with this as this PR unblocks #15599 (which gives us additional new good flow)