On Thu Oct 13 14:48:54 2022 +0000, Nikolay Sivov wrote:
Yes, I meant IMallocSpy part, usual concern is to look too much into implementation details. In this case it's really easy to do, and tracing API is of course public. But now that you know how to allocate/free correctly, you don't really need spying part. It's just my opinion, I'd wait for someone else to comment on this too.
Yep, the spy is there for two reasons. One is to "show my work" with a conformance test that demonstrates this is indeed supposed to be IMalloc. I.e. showing this implementation choice is externally-visible from documented interfaces. But it's probably enough that the MR shows reviewers that proof, it doesn't have to make it into the tree.
I could also have done this part with `IMalloc::DidAlloc`, though it's not a very good test since wine's implementation of that is semi-stub; HeapValidate will accept pointes that didn't come through IMalloc too. So I think the test work work as intended on windows, and it would pass on the fixed wine, but it would also false-positive pass on the still-broken wine. So that would be an option to document the intention (less invasively), though it's not actually a very sensitive regression test.
The other purpose is that also let me filter out the Free(0xdeadbeef) that could be pretty symmetrical to RecordInfo_RecordCopy. So if we drop it, we'd also have to switch the tests to actually-valid allocations (making them a little less sensitive to use-after-free or whether the record was supposed to be deep-copied or shallow-copied).
But I care more about the actual fix, I'm fine with whatever level of test coverage the you guys actually want to keep :-)