[cfe-dev] [ClangToolsExtra] PreprocessorTrackerImpl's use of llvm::StringPool

[Chris Lattner via cfe-dev]

Cool, thanks!  I put up this patch to replace this usage in favor of StringSet - once this goes in, I’ll remove StringPool entirely.  I’d appreciate an LGTM, thanks!

https://reviews.llvm.org/D78273 <https://reviews.llvm.org/D78273>

-Chris

> On Apr 15, 2020, at 11:17 AM, Fangrui Song <maskray at google.com> wrote:
> 
> On 2020-04-14, Reid Kleckner via cfe-dev wrote:
>> I also noticed StringPool last month and had more or less the same thought:
>> this looks dead, can we remove it?
>> 
>> +1 for replacing this usage with StringSet and removing StringPool.
> 
> +1. llvm/Support/StringSaver.h:UniqueStringSaver does more or less the
> same thing.
> 
>> On Mon, Apr 13, 2020 at 11:03 AM Chris Lattner via cfe-dev <
>> cfe-dev at lists.llvm.org> wrote:
>> 
>>> Cc John Thompson, but I don’t know his email address.
>>> 
>>> I recently ran across llvm::StringPool in llvm/lib/Support.  It is an old
>>> class which isn’t ideal in several ways, and I would like to remove it in
>>> favor of direct uses of llvm::StringSet, which is nearly the same thing and
>>> a lot more efficient.  The one difference is that in this one, each entry
>>> is individually reference counted, so they get deallocated as soon as the
>>> last reference is dropped.
>>> 
>>> The only use of this class is in PreprocessorTrackerImpl in
>>> clang-tools-extra/modularize/PreprocessorTracker.cpp.  Does anyone object
>>> to changing this to being a simple llvm::StringSet?  Such a change would
>>> make this more efficient (and allow deleting a misleading class out of
>>> llvm/lib/Support), allow simplifying away the "StringHandle” logic (in
>>> favor of using StringRef directly) and the only cost that I can see is that
>>> the strings won’t get deallocated as eagerly.
>>> 
>>> An alternative solution here is to move StringPool into clang-tools-extra
>>> as a private implementation detail.  I’d personally rather remove it
>>> outright than preserve it.
>>> 
>>> Any thoughts?
>>> 
>>> -Chris
>>> _______________________________________________
>>> cfe-dev mailing list
>>> cfe-dev at lists.llvm.org
>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>> 
> 
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20200415/a049584c/attachment.html>