[llvm-commits] [llvm] r97759 - /llvm/trunk/lib/CodeGen/PseudoSourceValue.cpp

[Jeffrey Yasskin]

On Thu, Mar 4, 2010 at 2:23 PM, Chris Lattner <clattner at apple.com> wrote:
>
> On Mar 4, 2010, at 2:15 PM, Jeffrey Yasskin wrote:
>
>> Author: jyasskin
>> Date: Thu Mar  4 16:15:01 2010
>> New Revision: 97759
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=97759&view=rev
>> Log:
>> Fix memcheck-found leaks: one false positive from using new[], and one true
>> positive where pointers would be leaked on llvm_shutdown.
>
> Nice, one question:
>
>> +namespace {
>> +struct PSVGlobalsTy {
>> +  // PseudoSourceValues are immutable so don't need locking.
>> +  const PseudoSourceValue PSVs[4];
>> +  sys::Mutex Lock;  // Guards FSValues, but not the values inside it.
>> +  std::map<int, const PseudoSourceValue *> FSValues;
>> +
>> +  PSVGlobalsTy() : PSVs() {}
>> +  ~PSVGlobalsTy() {
>> +    for (std::map<int, const PseudoSourceValue *>::iterator
>> +           I = FSValues.begin(), E = FSValues.end(); I != E; ++I) {
>> +      delete I->second;
>
> Why does this need an std::map?  Aren't the keys dense integers?  Why not directly index into the array, allowing elimination of the lock?

PseudoSourceValue::getFixedStack lets the user pass in an arbitrary
int. It could be that these tend to be dense, allowing us to use a
vector for FSValues instead of the map, but the vector would still
have to be growable to accommodate larger integers than it's seen yet,
which would still require a lock. Since I don't know how this is used,
I just rearranged it instead of trying to change the data structure or
move it into the Context.