[PATCH] LibDriver, llvm-lib: introduce.

Rui Ueyama ruiu at google.com
Wed Jun 10 16:40:34 PDT 2015


Looks like I don't fully understand the patch... Is there any reason you
can't simply define a non-thread-safe virtual StringSaver.save(String) in
llvm and not define StringSaverBase? LLD can then override the function to
guard it with a mutex.

On Wed, Jun 10, 2015 at 7:50 AM, Rafael EspĂ­ndola <
rafael.espindola at gmail.com> wrote:

> On 9 June 2015 at 23:32, Rui Ueyama <ruiu at google.com> wrote:
> > I think you still need a lock for LLD, but I'd rather not edit that code.
> > Once the new COFF port is ready to use, that code will be deleted
> entirely,
> > so maybe you want to just leave it alone.
>
> The disadvantage is that we need to keep the virtual interface. Oh
> well, we can drop that when that code goes away.
>
> > A few other comments on the patch.
> >
> > - StringSaver does one thing, so it has only one function: save(). Does
> it
> > make sense to make the class to overload operator() so that we can call
> > Saver(S) instead of Saver.save(S)?
>
> I think I prefer Saver.save(S).
>
> > - "Pass a pointer if an object could be mutated" is my personal coding
> style
> > and not everybody does that in LLVM. Which do we prefer,
> > StringSaver(BumpPtrAllocator *Alloc) or StringSaver(BumpPtrAllocator
> &Alloc)
> > as a constructor's signature?
>
> My personal style is use a reference if it is not null and will not be
> deleted. Unfortunately that is an area that is fairly inconsistent in
> LLVM.
>
> The attached patches keep the virtual interface but avoids the virtual
> destructor, makes the virtual interface protected and passes a
> StringRef to it.
>
> Cheers,
> Rafael
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150610/c42d5bc1/attachment.html>


More information about the llvm-commits mailing list