DenseMap did not copy complete buckets when moving, growing or copying. This patch makes that happen.
You need to add unit tests to show what the bug is, and how this change is fixing it. That should be added as a test case in the existing unit test for DenseMap. Right now I'm inclined to reject this change given the issue raised inline.
This smells funny -- first you destroy the object, then call a function on the destroyed object. I'm positive you are running into undefined behaviour here. To fix this, you should attempt to re-initialize the object pointed to through P using placement new.
This is no more smelly than the initial problems. if you look at the source code, the BucketTs are not initialized anywhere. There is never a constructor called on them. This is no different to how they are initialized initially. See line 326, in the file.
I am just making it less bad. Really we need to initialize the buckets correctly, however this requires more work than I can spare.
As Dean said,
- You need a very clear testcase demonstrating the issue you are trying to solve.
- A clear description how the change solves the problem.
What it looks like now is "a change people are having trouble understanding the reasoning for" and "an argument that world is broken right now".
That is leaving people confused.
Saying "I am just making it less bad. Really we need to initialize the buckets correctly, however this requires more work than I can spare." doesn't help.
It's okay if you don't have time to fix it right.
File a bug, with a reproducible testcase demonstrating what's wrong, and someone who does have time will probably take a stab at it.
In the meantime, we probably don't want to hack up one of the most used datastructures in LLVM with an incompletely solution to a problem people don't understand yet.