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

[Zachary Turner via llvm-commits]

Does the char array constructor get selected in this case?

char Buf[MAX_PATH];
sprintf(Buf, "%s/%s", Dir, Folder);
StringRef S(Buf);

On Mon, Oct 17, 2016 at 12:38 PM Mehdi Amini <mehdi.amini at apple.com> wrote:

> On Oct 17, 2016, at 10:12 AM, David Blaikie <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.)
>
>
> The balance is between the readability, and the “error prone” current
> behavior.
>
> For example we introduced all these redundant .c_str() in the past:
> https://reviews.llvm.org/D25667
> Ideally we shouldn’t have much explicit calls to StringRef, if all the
> APIs are converted to work with StringRef most of the time.
>
>
> Are these changes necessarily joined - or is the explicitness of the
> StringRef(...) ctor independent of the addition of the template?
>
>
> It is not independent: the template is never selected if the other
> constructor is not made explicit.
>
>
> 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?
>
>
> Can you clarify what you mean by "non-constexpr context” here?
> The recursive template is not intended to be used with anything else than
> literal, I am not expecting this to generate any code ever (if it does, it
> is not intended).
>
> —
> Mehdi
>
>
>
>
> On Mon, Oct 17, 2016 at 12:57 AM Manuel Klimek via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
> klimek added a comment.
>
> In 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
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> 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/a9128180/attachment.html>