[PATCH] D25645: [ADT] Add CachedHashString.

[Justin Lebar via llvm-commits]

jlebar added a comment.

In https://reviews.llvm.org/D25645#576823, @mehdi_amini wrote:

> In https://reviews.llvm.org/D25645#574862, @mehdi_amini wrote:
>
> > Couldn't `CachedHashString` just wrap a `CachedHashStringRef`?
> >
> > Or even have a single implementation:
> >
> >   template<bool Owning>
> >   class CachedHashStringRefBase {
> >     ...
> >  
> >     ~CachedHashStringRefBase() {
> >       if (owning) delete...
> >   }
> >   using CachedHashStringRef = CachedHashStringRefBase<false>;
> >   using CachedHashString = CachedHashStringRefBase<true>;
> >  
> >
>
>
> Ping?


Sorry, forgot to respond to this.

I don't think this is a good idea, because the two classes do not have the same API.  In particular, CachedHashString has a conversion to CachedHashStringRef.  We would have to enable-if this away.

Also the bodies of about half of the functions would be different.  At this point we would not be gaining much from this additional layer of indirection.


Repository:
  rL LLVM

https://reviews.llvm.org/D25645