[PATCH] D25257: Use StringRef in Option library instead of raw pointers (NFC)
Mehdi AMINI via llvm-commits
llvm-commits at lists.llvm.org
Tue Oct 4 16:38:01 PDT 2016
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
More information about the llvm-commits
mailing list