[PATCH] D25257: Use StringRef in Option library instead of raw pointers (NFC)

[Mehdi AMINI via llvm-commits]

mehdi_amini added a comment.

In https://reviews.llvm.org/D25257#561642, @zturner wrote:

> but since it is re-written it has the potential to introduce new bugs.


Yes, I spent 2 days debugging this one because of subtleties...

> As a result, I've found smaller CLs to be much more workable.

This is one patch in a ~40 patches queue, I let you imagine how much smaller it is from the main diff :)

> Did you find that it was impossible to do this in a smaller CL?

It is possible, I have to go through intermediate stages where I have a lot of conversion through `.data()` for instance. So when you have something working it is quite hard to go back.
Also the story of this patch is that I have a queue of patches to update all the LLVM APIs, and now I update the other subprojects (clang, lld, lldb) before committing these individual patches. So the question is "how do I proceed?". What depth do I propagate the change?

>   For example, the first change I see is  changing this:
> 
>   const char *addTempFile(const char *Name)
> 
> 
> to this:
> 
>   llvm::StringRef addTempFile(llvm::StringRef Name)
> 
> 
> If instead you started from a blank slate and changed it to this:
> 
>   llvm::StringRef addTempFile(const char *Name)
> 
> 
> You would get a lot of immediate compilation failures, which you could fix up and (theoretically) end up with a CL much smaller than what you have here.  Did you try that and find it impractical for some reason?

I started from a patch ready in LLVM that convert the Option library. So the issue is as I update the clang codebase to adapt for the change, there a some minor API like the one you're mentioning that come up. I can "stash" my current patch and deal with this API, before coming back to the current patch. However that's not practical: I have to rebuild the full codebase every time and it takes *long* (I only have a laptop for that, not a large workstation).

As mentioned above, if you think it is better, I can go through intermediate stages where I just add a bunch of `.data()` conversion. But this intermediate stage will not look pretty.


https://reviews.llvm.org/D25257