[lld] r230219 - [ELF][Writer] Use llvm::StringMap instead

[Shankar Easwaran]

On 2/23/2015 11:44 AM, David Blaikie wrote:
> On Mon, Feb 23, 2015 at 9:43 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>> On Mon, Feb 23, 2015 at 9:32 AM, Shankar Easwaran <shankare at codeaurora.org
>>> wrote:
>>> On 2/23/2015 10:56 AM, David Blaikie wrote:
>>>
>>>> On Mon, Feb 23, 2015 at 5:50 AM, Shankar Easwaran <
>>>> shankare at codeaurora.org>
>>>> wrote:
>>>>
>>>>   Author: shankare
>>>>> Date: Mon Feb 23 07:50:23 2015
>>>>> New Revision: 230219
>>>>>
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=230219&view=rev
>>>>> Log:
>>>>> [ELF][Writer] Use llvm::StringMap instead
>>>>>
>>>>>   This does change the semantics somewhat - StringMaps own their
>>>> strings. do
>>>> you need that? (if so, is there a test case for where the map<StringRef
>>>> was
>>>> insufficient?) Or does this use case neither require nor forbid copying
>>>> the
>>>> strings, and just benefit from the StringMap optimizations in memory,
>>>> etc?
>>>>
>>>>   Thanks for the review!.
>>> Oh, I dont want StringMaps to start copying the strings and own it. Is
>>> the alternative to use llvm::DenseMap then ?
>>>
>>> The reason I dont want to use std::map was for performance.
>>
>> If it's performance, it's possible that copying the strings is still
>> better than not - your perf experiments/numbers should show if it's the
>> wrong tradeoff.
>>
>> But yeah, if you're running some experiments, it might be worth trying a
>> DenseMap to see if non-owning StringRefs are the better tradeoff.
>>
> Oh, and if it turns out the perf tradeoff is in favor of StringMap, it
> might be worth a comment mentioning that the ownership isn't needed in this
> use case (in case someone wants to refactor this again later).
Sure, will do. Thanks for the review!

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation