[PATCH] D25255: Add a c_str() method to StringRef, similar to data() but asserting if the string isn't null terminated (NFC)

[Zachary Turner via llvm-commits]

I'd be more comfortable if the name was like unsafe_cstr() or something.
Thoughts?
On Tue, Oct 4, 2016 at 4:51 PM Mehdi Amini <mehdi.amini at apple.com> wrote:

> On Oct 4, 2016, at 4:46 PM, Zachary Turner <zturner at google.com> wrote:
>
> I mean "how many callsites are there”?
>
>
> I haven’t audited / counted, and I’m not done with all the conversions yet.
>
>
>
> On Tue, Oct 4, 2016 at 4:23 PM Mehdi Amini <mehdi.amini at apple.com> wrote:
>
> On Oct 4, 2016, at 4:17 PM, Zachary Turner <zturner at google.com> wrote:
>
> How frequently would it need to be done at each callsite?
>
>
> I don’t understand the question. Basically “always”?
>
>
> My concern is just that it sounds like we're adding a potentially
> dangerous function to a very generic and widely used API for the purposes
> of one client.
>
>
> Well the alternative to this API is .data(), the goal is to add a safer
> API than .data() :)
>
> I'd inspect closely any client that is using .data() instead of this one.
>
>
>
>   If you make the function available, people are going to use it without
> understanding the implications.  Another possibility is that if this all
> takes place within a single common translation unit, you could write a
> private function inside there that looks like this:
>
> const char *Unsafe_cstr(StringRef S) {
>   assert(strlen(S.data()) == S.size());
>   return S.data();
> }
>
> That way the unsafe function is not exposed to general clients of
> StringRef?
>
>
> On Tue, Oct 4, 2016 at 4:10 PM Mehdi Amini <mehdi.amini at apple.com> wrote:
>
> The use case is that MachineInstruction have operands, but the size of
> their operands is limited. Right now you have the space for one-pointer,
> but not for a StringRef. So if you create a MachineInstruction that takes a
> symbol name as an operand, we can only store the “const chat *” from the
> StringRef.
>
> So what you’re suggesting to do is roughly what I’m trying to achieve here
> as well, but instead of replicating the logic at each call sites, I try to
> abstract it behind the API.
>
>
> On Oct 4, 2016, at 3:56 PM, Zachary Turner <zturner at google.com> wrote:
>
> I'm not familiar with those cases, but could you do something like this:
>
> assert(strlen(s.data()) == s.size() && "Str is not null terminated!");
>
> at each call site?
>
>
> On Tue, Oct 4, 2016 at 3:48 PM Mehdi Amini <mehdi.amini at apple.com> wrote:
>
> Neat, I didn’t know this one!
>
> That does not solve cases like the round-trip StringRef -> MO -> StringRef
> though.
>
>
> On Oct 4, 2016, at 3:38 PM, Zachary Turner <zturner at google.com> wrote:
>
> Can you use %*s format specifier in those cases?
>
> printf("%*s", s.size(), s.data());
>
> On Tue, Oct 4, 2016 at 3:34 PM Mehdi AMINI <mehdi.amini at apple.com> wrote:
>
> mehdi_amini added a comment.
>
> In https://reviews.llvm.org/D25255#561566, @zturner wrote:
>
> > Not sure how I feel about this.  It's convenient, but it has potential
> for abuse.  Where did you run into issues porting code to `StringRef` that
> this solves?
>
>
> I ran into this with "printf" like API at some point, or with places that
> can't be converted to StringRef because of space constraint, like
> MachineOperand. So you're out of the IR and you have to use .data() to
> initialize the MO. Later you may construct a StringRef from this "const
> char *" again.
>
> >   I've done a lot of `StringRef` porting in LLDB by now, and I've always
> managed to find a solution to this.  Usually it involves trickling the
> `StringRef` changes down further
>
> Usually that's what I do, yes. The problem is when you reach some cases
> like above.
>
>
> https://reviews.llvm.org/D25255
>
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161004/3d7585e7/attachment.html>