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
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
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
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
>
>
>
>
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
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
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
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
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
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
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
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
zturner created this revision.
zturner added a reviewer: clayborg.
zturner added a subscriber: lldb-commits.
As before, some copies are removed, a few are introduced. When I get to
Option::SetValue, those that are introduced should go away.
https://reviews.llvm.org/D24847
Files:
include/lldb
13 matches
Mail list logo
