[llvm-commits] [llvm] r147090 - /llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp

[David Blaikie]

(I rather like strict const but it is apparently a deliberate non goal of
the code base so I'd say it's probably idiomatic to just revert)
------------------------------
From: Chandler Carruth
Sent: 12/21/2011 12:36 PM
To: Jakub Staszak
Cc: llvm-commits at cs.uiuc.edu; David Blaikie
Subject: Re: [llvm-commits] [llvm] r147090 -
/llvm/trunk/lib/CodeGen/MachineBlockPlacement.cpp

On Wed, Dec 21, 2011 at 2:34 PM, Jakub Staszak <kubastaszak at gmail.com>wrote:

> I have to agree with operator[] vs lookup(). We don't get any benefit
> here, so there is no point to make code less readable. In this case I'd
> have to revert the whole patch, because most of these operator[] won't work
> because of its lack of constantness.
>

For reference, I'm fine with that. I don't think constantness is helping
this code.


>
> - Kuba
>
> On Dec 21, 2011, at 11:11 PM, Chandler Carruth wrote:
>
> On Wed, Dec 21, 2011 at 2:00 PM, Jakub Staszak <kubastaszak at gmail.com>wrote:
>
>> operator[] inserts an object for key if it doesn't exist.
>>
>> // X is empty
>> if (X[0] == a) { }
>> if (X[1] == b) { }
>> if (X[2] == c) { }
>> // X.size() = 3
>>
>> This is the way that std::map works. I believe we want to be quite
>> compatible here.
>
>
> Certainly, but is it actually cheaper if you know ahead of time that no
> such insertion will occur? I would expect the lookup path to a hot path
> through the [] operators.
>
> To be honest, I'm beginning to question the entire patch. No insertion
> ever happens with these [] uses, and they seem more readable to me than
> lookup... I don't feel strongly about the lookup, but I do feel strongly
> about the const thing. I'd really rather you strip back out all the const
> here.
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20111221/0420715c/attachment.html>