[llvm] r204431 - [Constant Hoisting] Replace the MapVector with a separate Map and Vector to keep track of constant candidates.

David Blaikie dblaikie at gmail.com
Fri Mar 21 13:54:02 PDT 2014


On Fri, Mar 21, 2014 at 10:56 AM, Juergen Ributzka <juergen at apple.com> wrote:
> This isn't about functionality (mostly). I did this mostly to separate the
> map and the vector, because the mapping will get invalidated when the vector
> gets sorted.

Is MapVector's API broken by exposing the ability to break its
invariants like this? Seems like it really shouldn't allow non-const
access to the keys... especially since they're duplicated in the map
and vector so they won't be kept in sync.

Perhaps it'd be nice to fix it, so this kind of confusing code can't be written?

>If someone later on modifies the code and doesn't know or
> missed the fact that the mapping is no longer valid, then they may use the
> MapVector incorrectly.

Would it be useful to invalidate the map and/or vector when this sort
operation occurs? Perhaps emptying both the map and vector (simply
std::move the vector into a local, sort it, use it, then leave the
function with both map and vector empty - since leaving them non-empty
is just a trap for someone later?)


- David

>
> -Juergen
>
> On Mar 21, 2014, at 2:23 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
> I don't really understand why the MapVector wasn't sufficient - what's
> the functionality that MapVector didn't provide that you are
> leveraging from a separate Map and Vector?
>
>



More information about the llvm-commits mailing list