[PATCH] D25639: Add ctor for string literal to StringRef, and make explicit the conversion from const char *

[Pete Cooper via llvm-commits]

> On Oct 17, 2016, at 10:20 AM, Zachary Turner <zturner at google.com> wrote:
> 
> I believe the idea is that all the extraneous explicit StringRef constructions would go away over time as global tables are updated to hold StringRefs, functions are updated to stop returning const char *, etc.
Not sure how many such tables exist, but we may not want to take the binary size hit of updating global tables.  Each entry will double in size from one pointer to 2.

Of course that will reduce the run time because then no strlen calls, so there’s a balance to be had.  Just something to keep an eye on for any of the larger tables.

Pete
> 
> With that in mind, it might be worth getting appropriate clang-tidy checks upstreamed. For example, if someone changes a method from returning a const char * to returning a StringRef, existing code that wrote foo(StringRef(bar()) could be rewritten as just foo(bar()), but no warning or error would be issued.
> 
> So having a check like --llvm-unnecessary-stringref-construction would make sure we can remove these as we are able to
> On Mon, Oct 17, 2016 at 10:12 AM David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
> (for my money the readability loss of adding StringRef(...) all over the place doesn't seem quite worth it, but I'm not going to stand in the way of the change or anything.)
> 
> Are these changes necessarily joined - or is the explicitness of the StringRef(...) ctor independent of the addition of the template?
> 
> Do we need to worry about the recursive strlen in the template ctor? Or is it clear that even in a non-constexpr context the compilers we care about produce reasonable performance?
> 
> On Mon, Oct 17, 2016 at 12:57 AM Manuel Klimek via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
> klimek added a comment.
> 
> In https://reviews.llvm.org/D25639#571344 <https://reviews.llvm.org/D25639#571344>, @zturner wrote:
> 
> > The only thing I'm not crazy about here is that the clang tidy check seems to blindly replace all calls to `s.c_str()` with `llvm::StringRef(s.c_str())`.  Is there any way to make it attempt to replace it with just `s` first, and only if that fails do you then try `llvm::StringRef(s.c_str())`?
> 
> 
> Shouldn't it be relatively straight forward to discover whether the expression is convertible to StringRef without the .c_str() call from an AST matcher?
> 
> 
> https://reviews.llvm.org/D25639 <https://reviews.llvm.org/D25639>
> 
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161017/e947e610/attachment.html>