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