Page MenuHomePhabricator

Make OptionGroup::SetValue take a StringRef
ClosedPublic

Authored by zturner on Sep 22 2016, 2:38 PM.

Details

Summary

As before, some copies are removed, a few are introduced. When I get to Option::SetValue, those that are introduced should go away.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 72216.Sep 22 2016, 2:38 PM
zturner retitled this revision from to Make OptionGroup::SetValue take a StringRef.
zturner updated this object.
zturner added a reviewer: clayborg.
zturner added a subscriber: lldb-commits.
clayborg requested changes to this revision.Sep 22 2016, 3:14 PM
clayborg edited edge metadata.

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_STRINGREF_PRINTF_ARGS(s) (int)s.size(), s.data()

Then when doing errors or anything that does printf:

error.SetErrorStringWithFormat("invalid all-threads value setting: \"" LLVM_STRINGREF_PRINTF_FORMAT "\"", LLVM_STRINGREF_PRINTF_ARGS(option_arg));

There are a few places where we should plumb through StringRef variants of calls, only 2, so I think it is worth including in this change.

source/Commands/CommandObjectRegister.cpp
269 ↗(On Diff #72216)

Add a StringRef variant as inside of OptionValueUInt64 it can use the StringRef::getAsInteger().

source/Interpreter/OptionGroupVariable.cpp
104 ↗(On Diff #72216)

Please add llvm::StringRef version of SetCurrentValue to OptionValueString:

Error
OptionValueString::SetCurrentValue(const llvm::StringRef &value);
107 ↗(On Diff #72216)

Please add llvm::StringRef version of SetCurrentValue to OptionValueString:

This revision now requires changes to proceed.Sep 22 2016, 3:14 PM

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.

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 just add the two quick ones.

no printf fixes are fine. I don't mind if error cases have str().c_str() so much.

You can still leave the "const char *" versions in there for now until you get to the cleanup. No spiral if you do that.

zturner updated this revision to Diff 72234.Sep 22 2016, 8:18 PM
zturner edited edge metadata.

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.

clayborg accepted this revision.Sep 23 2016, 9:57 AM
clayborg edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Sep 23 2016, 9:57 AM
This revision was automatically updated to reflect the committed changes.