Re: [Lldb-commits] [PATCH] D24847: Make OptionGroup::SetValue take a StringRef

2016-09-23 Thread Zachary Turner via lldb-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL282265: Update OptionGroup::SetValue to take StringRef. (authored by zturner). Changed prior to commit: https://reviews.llvm.org/D24847?vs=72234&id=72310#toc Repository: rL LLVM https://reviews.llvm

Re: [Lldb-commits] [PATCH] D24847: Make OptionGroup::SetValue take a StringRef

2016-09-23 Thread Greg Clayton via lldb-commits
clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Looks good. https://reviews.llvm.org/D24847 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/l

Re: [Lldb-commits] [PATCH] D24847: Make OptionGroup::SetValue take a StringRef

2016-09-22 Thread Zachary Turner via lldb-commits
zturner updated this revision to Diff 72234. zturner added a comment. Updated the two requested functions. I still had to make a few more changes to catch the implicit conversions into each of these functions, but it wasn't too bad. https://reviews.llvm.org/D24847 Files: include/lldb/Inter

Re: [Lldb-commits] [PATCH] D24847: Make OptionGroup::SetValue take a StringRef

2016-09-22 Thread Zachary Turner via lldb-commits
Ok that's fine then On Thu, Sep 22, 2016 at 3:59 PM Greg Clayton wrote: > clayborg added a comment. > > You can still leave the "const char *" versions in there for now until you > get to the cleanup. No spiral if you do that. > > > https://reviews.llvm.org/D24847 > > > >

Re: [Lldb-commits] [PATCH] D24847: Make OptionGroup::SetValue take a StringRef

2016-09-22 Thread Greg Clayton via lldb-commits
clayborg added a comment. You can still leave the "const char *" versions in there for now until you get to the cleanup. No spiral if you do that. https://reviews.llvm.org/D24847 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://list

Re: [Lldb-commits] [PATCH] D24847: Make OptionGroup::SetValue take a StringRef

2016-09-22 Thread Zachary Turner via lldb-commits
I took a quick glance, now I remember. It uses a validator, so the validator has to either update its signature or make a copy. It's also called by SetValueFromCString, so that has to be updated or make a copy. Then it spirals out from there. So yea, the copy is still going to be there, just in a

Re: [Lldb-commits] [PATCH] D24847: Make OptionGroup::SetValue take a StringRef

2016-09-22 Thread Greg Clayton via lldb-commits
clayborg added a comment. no printf fixes are fine. I don't mind if error cases have str().c_str() so much. https://reviews.llvm.org/D24847 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lld

Re: [Lldb-commits] [PATCH] D24847: Make OptionGroup::SetValue take a StringRef

2016-09-22 Thread Greg Clayton via lldb-commits
clayborg added a comment. Yeah, don't do any calls that don't need to be converted. Just the ones you need. Should just add 2 StringRef variant functions. Don't feel the need to completely fix OptionValueString or OptionValueUInt64. We can do the full change over in a future CL. See if you can

Re: [Lldb-commits] [PATCH] D24847: Make OptionGroup::SetValue take a StringRef

2016-09-22 Thread Zachary Turner via lldb-commits
I'll try the 2 additional string ref changes first and see how bad it is On Thu, Sep 22, 2016 at 3:36 PM Zachary Turner wrote: > OptionValueString is actually the next item on my list. I tried it before > this and it was a very big CL, But maybe with this done it will be smaller. > > I can try ag

Re: [Lldb-commits] [PATCH] D24847: Make OptionGroup::SetValue take a StringRef

2016-09-22 Thread Zachary Turner via lldb-commits
OptionValueString is actually the next item on my list. I tried it before this and it was a very big CL, But maybe with this done it will be smaller. I can try again but if the cl grows too large i think it's better to do it in a followup. If nothing else so that if a buildbot fails it's easier to

Re: [Lldb-commits] [PATCH] D24847: Make OptionGroup::SetValue take a StringRef

2016-09-22 Thread Greg Clayton via lldb-commits
clayborg added a comment. I am not saying we have to do the printf changes, I was just seeing what you think. I would like to see the StringRef variants of functions put in as part of this. https://reviews.llvm.org/D24847 ___ lldb-commits mailing

Re: [Lldb-commits] [PATCH] D24847: Make OptionGroup::SetValue take a StringRef

2016-09-22 Thread Greg Clayton via lldb-commits
clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. We could avoid many of the copies in the printf statements by by doing something like this in a common header file: #define LLVM_STRINGREF_PRINTF_FORMAT "%*s" #define LLVM_S