Re: [Lldb-commits] [PATCH] D24847: Make OptionGroup::SetValue take a StringRef
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.org/D24847 Files: lldb/trunk/include/lldb/Interpreter/Args.h lldb/trunk/include/lldb/Interpreter/OptionGroupArchitecture.h lldb/trunk/include/lldb/Interpreter/OptionGroupBoolean.h lldb/trunk/include/lldb/Interpreter/OptionGroupFile.h lldb/trunk/include/lldb/Interpreter/OptionGroupFormat.h lldb/trunk/include/lldb/Interpreter/OptionGroupOutputFile.h lldb/trunk/include/lldb/Interpreter/OptionGroupPlatform.h lldb/trunk/include/lldb/Interpreter/OptionGroupString.h lldb/trunk/include/lldb/Interpreter/OptionGroupUInt64.h lldb/trunk/include/lldb/Interpreter/OptionGroupUUID.h lldb/trunk/include/lldb/Interpreter/OptionGroupValueObjectDisplay.h lldb/trunk/include/lldb/Interpreter/OptionGroupVariable.h lldb/trunk/include/lldb/Interpreter/OptionGroupWatchpoint.h lldb/trunk/include/lldb/Interpreter/OptionValue.h lldb/trunk/include/lldb/Interpreter/OptionValueArch.h lldb/trunk/include/lldb/Interpreter/OptionValueArray.h lldb/trunk/include/lldb/Interpreter/OptionValueBoolean.h lldb/trunk/include/lldb/Interpreter/OptionValueChar.h lldb/trunk/include/lldb/Interpreter/OptionValueDictionary.h lldb/trunk/include/lldb/Interpreter/OptionValueEnumeration.h lldb/trunk/include/lldb/Interpreter/OptionValueFileSpec.h lldb/trunk/include/lldb/Interpreter/OptionValueFileSpecList.h lldb/trunk/include/lldb/Interpreter/OptionValueFormat.h lldb/trunk/include/lldb/Interpreter/OptionValueFormatEntity.h lldb/trunk/include/lldb/Interpreter/OptionValueLanguage.h lldb/trunk/include/lldb/Interpreter/OptionValuePathMappings.h lldb/trunk/include/lldb/Interpreter/OptionValueProperties.h lldb/trunk/include/lldb/Interpreter/OptionValueRegex.h lldb/trunk/include/lldb/Interpreter/OptionValueSInt64.h lldb/trunk/include/lldb/Interpreter/OptionValueString.h lldb/trunk/include/lldb/Interpreter/OptionValueUInt64.h lldb/trunk/include/lldb/Interpreter/OptionValueUUID.h lldb/trunk/include/lldb/Interpreter/Options.h lldb/trunk/include/lldb/Target/Language.h lldb/trunk/include/lldb/Target/Platform.h lldb/trunk/source/API/SBLanguageRuntime.cpp lldb/trunk/source/Commands/CommandObjectBreakpoint.cpp lldb/trunk/source/Commands/CommandObjectBreakpointCommand.cpp lldb/trunk/source/Commands/CommandObjectCommands.cpp lldb/trunk/source/Commands/CommandObjectExpression.cpp lldb/trunk/source/Commands/CommandObjectExpression.h lldb/trunk/source/Commands/CommandObjectMemory.cpp lldb/trunk/source/Commands/CommandObjectPlatform.cpp lldb/trunk/source/Commands/CommandObjectRegister.cpp lldb/trunk/source/Commands/CommandObjectTarget.cpp lldb/trunk/source/Commands/CommandObjectThread.cpp lldb/trunk/source/Commands/CommandObjectType.cpp lldb/trunk/source/Commands/CommandObjectWatchpointCommand.cpp lldb/trunk/source/Interpreter/Args.cpp lldb/trunk/source/Interpreter/OptionGroupArchitecture.cpp lldb/trunk/source/Interpreter/OptionGroupBoolean.cpp lldb/trunk/source/Interpreter/OptionGroupFile.cpp lldb/trunk/source/Interpreter/OptionGroupFormat.cpp lldb/trunk/source/Interpreter/OptionGroupOutputFile.cpp lldb/trunk/source/Interpreter/OptionGroupPlatform.cpp lldb/trunk/source/Interpreter/OptionGroupString.cpp lldb/trunk/source/Interpreter/OptionGroupUInt64.cpp lldb/trunk/source/Interpreter/OptionGroupUUID.cpp lldb/trunk/source/Interpreter/OptionGroupValueObjectDisplay.cpp lldb/trunk/source/Interpreter/OptionGroupVariable.cpp lldb/trunk/source/Interpreter/OptionGroupWatchpoint.cpp lldb/trunk/source/Interpreter/OptionValue.cpp lldb/trunk/source/Interpreter/OptionValueChar.cpp lldb/trunk/source/Interpreter/OptionValueDictionary.cpp lldb/trunk/source/Interpreter/OptionValueString.cpp lldb/trunk/source/Interpreter/OptionValueUInt64.cpp lldb/trunk/source/Interpreter/Options.cpp lldb/trunk/source/Interpreter/Property.cpp lldb/trunk/source/Target/Language.cpp lldb/trunk/source/Target/Platform.cpp Index: lldb/trunk/source/Target/Platform.cpp === --- lldb/trunk/source/Target/Platform.cpp +++ lldb/trunk/source/Target/Platform.cpp @@ -1401,7 +1401,7 @@ lldb_private::Error OptionGroupPlatformRSync::SetOptionValue(uint32_t option_idx, - const char *option_arg, + llvm::StringRef option_arg, ExecutionContext *execution_context) { Error error; char short_option = (char)GetDefinitions()[option_idx].short_option; @@ -1447,7 +1447,7 @@ lldb_private::Error OptionGroupPlatformSSH::SetOptionValue(uint32_t option_idx, - const char *option_arg, +
Re: [Lldb-commits] [PATCH] D24847: Make OptionGroup::SetValue take a StringRef
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/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24847: Make OptionGroup::SetValue take a StringRef
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/Interpreter/Args.h include/lldb/Interpreter/OptionGroupArchitecture.h include/lldb/Interpreter/OptionGroupBoolean.h include/lldb/Interpreter/OptionGroupFile.h include/lldb/Interpreter/OptionGroupFormat.h include/lldb/Interpreter/OptionGroupOutputFile.h include/lldb/Interpreter/OptionGroupPlatform.h include/lldb/Interpreter/OptionGroupString.h include/lldb/Interpreter/OptionGroupUInt64.h include/lldb/Interpreter/OptionGroupUUID.h include/lldb/Interpreter/OptionGroupValueObjectDisplay.h include/lldb/Interpreter/OptionGroupVariable.h include/lldb/Interpreter/OptionGroupWatchpoint.h include/lldb/Interpreter/OptionValue.h include/lldb/Interpreter/OptionValueArch.h include/lldb/Interpreter/OptionValueArray.h include/lldb/Interpreter/OptionValueBoolean.h include/lldb/Interpreter/OptionValueChar.h include/lldb/Interpreter/OptionValueDictionary.h include/lldb/Interpreter/OptionValueEnumeration.h include/lldb/Interpreter/OptionValueFileSpec.h include/lldb/Interpreter/OptionValueFileSpecList.h include/lldb/Interpreter/OptionValueFormat.h include/lldb/Interpreter/OptionValueFormatEntity.h include/lldb/Interpreter/OptionValueLanguage.h include/lldb/Interpreter/OptionValuePathMappings.h include/lldb/Interpreter/OptionValueProperties.h include/lldb/Interpreter/OptionValueRegex.h include/lldb/Interpreter/OptionValueSInt64.h include/lldb/Interpreter/OptionValueString.h include/lldb/Interpreter/OptionValueUInt64.h include/lldb/Interpreter/OptionValueUUID.h include/lldb/Interpreter/Options.h include/lldb/Target/Language.h include/lldb/Target/Platform.h source/API/SBLanguageRuntime.cpp source/Commands/CommandObjectBreakpoint.cpp source/Commands/CommandObjectBreakpointCommand.cpp source/Commands/CommandObjectCommands.cpp source/Commands/CommandObjectExpression.cpp source/Commands/CommandObjectExpression.h source/Commands/CommandObjectMemory.cpp source/Commands/CommandObjectPlatform.cpp source/Commands/CommandObjectRegister.cpp source/Commands/CommandObjectTarget.cpp source/Commands/CommandObjectThread.cpp source/Commands/CommandObjectType.cpp source/Commands/CommandObjectWatchpointCommand.cpp source/Interpreter/Args.cpp source/Interpreter/OptionGroupArchitecture.cpp source/Interpreter/OptionGroupBoolean.cpp source/Interpreter/OptionGroupFile.cpp source/Interpreter/OptionGroupFormat.cpp source/Interpreter/OptionGroupOutputFile.cpp source/Interpreter/OptionGroupPlatform.cpp source/Interpreter/OptionGroupString.cpp source/Interpreter/OptionGroupUInt64.cpp source/Interpreter/OptionGroupUUID.cpp source/Interpreter/OptionGroupValueObjectDisplay.cpp source/Interpreter/OptionGroupVariable.cpp source/Interpreter/OptionGroupWatchpoint.cpp source/Interpreter/OptionValue.cpp source/Interpreter/OptionValueChar.cpp source/Interpreter/OptionValueDictionary.cpp source/Interpreter/OptionValueString.cpp source/Interpreter/OptionValueUInt64.cpp source/Interpreter/Options.cpp source/Interpreter/Property.cpp source/Target/Language.cpp source/Target/Platform.cpp Index: source/Target/Platform.cpp === --- source/Target/Platform.cpp +++ source/Target/Platform.cpp @@ -1401,7 +1401,7 @@ lldb_private::Error OptionGroupPlatformRSync::SetOptionValue(uint32_t option_idx, - const char *option_arg, + llvm::StringRef option_arg, ExecutionContext *execution_context) { Error error; char short_option = (char)GetDefinitions()[option_idx].short_option; @@ -1447,7 +1447,7 @@ lldb_private::Error OptionGroupPlatformSSH::SetOptionValue(uint32_t option_idx, - const char *option_arg, + llvm::StringRef option_arg, ExecutionContext *execution_context) { Error error; char short_option = (char)GetDefinitions()[option_idx].short_option; @@ -1478,7 +1478,7 @@ } lldb_private::Error OptionGroupPlatformCaching::SetOptionValue( -uint32_t option_idx, const char *option_arg, +uint32_t option_idx, llvm::StringRef option_arg, ExecutionContext *execution_context) { Error error; char short_option = (char)GetDefinitions()[option_idx].short_option; Index: source/Target/Language.cpp === --- source/Target/Language.cpp +++ source/Target/Language.cpp @@ -171,10 +171,6 @@ static uint32_t num_languages = sizeof(language_names) / sizeof(struct language_name_pair);
Re: [Lldb-commits] [PATCH] D24847: Make OptionGroup::SetValue take a StringRef
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 > > > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24847: Make OptionGroup::SetValue take a StringRef
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://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24847: Make OptionGroup::SetValue take a StringRef
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 different place until there's quite a bit more plumbing done On Thu, Sep 22, 2016 at 3:45 PM Greg Clayton wrote: > 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/lldb-commits
Re: [Lldb-commits] [PATCH] D24847: Make OptionGroup::SetValue take a StringRef
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/lldb-commits
Re: [Lldb-commits] [PATCH] D24847: Make OptionGroup::SetValue take a StringRef
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 just add the two quick ones. https://reviews.llvm.org/D24847 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24847: Make OptionGroup::SetValue take a StringRef
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 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 > track it down with a more modest sized cl. > > For the printing stuff, it might be cleaner to just use an llvm stream and > then pass it to Error::SetErrorString. The %*s macros will look kind of > gross (for the same reason that the PRIx64 style macros look so horrible). > > But all these things combined will make a really big CL, I'd rather do it > in chunks. Maybe format strings next then OptionValueString? > On Thu, Sep 22, 2016 at 3:15 PM Greg Clayton wrote: > > 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 list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24847: Make OptionGroup::SetValue take a StringRef
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 track it down with a more modest sized cl. For the printing stuff, it might be cleaner to just use an llvm stream and then pass it to Error::SetErrorString. The %*s macros will look kind of gross (for the same reason that the PRIx64 style macros look so horrible). But all these things combined will make a really big CL, I'd rather do it in chunks. Maybe format strings next then OptionValueString? On Thu, Sep 22, 2016 at 3:15 PM Greg Clayton wrote: > 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 list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24847: Make OptionGroup::SetValue take a StringRef
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 list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24847: Make OptionGroup::SetValue take a StringRef
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_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. Comment at: source/Commands/CommandObjectRegister.cpp:269 @@ -268,3 +268,3 @@ case 's': { -OptionValueSP value_sp(OptionValueUInt64::Create(option_value, error)); +OptionValueSP value_sp(OptionValueUInt64::Create(option_value.str().c_str(), error)); if (value_sp) Add a StringRef variant as inside of OptionValueUInt64 it can use the StringRef::getAsInteger(). Comment at: source/Interpreter/OptionGroupVariable.cpp:104 @@ -103,3 +103,3 @@ case 'y': -error = summary.SetCurrentValue(option_arg); +error = summary.SetCurrentValue(option_arg.str().c_str()); break; Please add llvm::StringRef version of SetCurrentValue to OptionValueString: ``` Error OptionValueString::SetCurrentValue(const llvm::StringRef &value); ``` Comment at: source/Interpreter/OptionGroupVariable.cpp:107 @@ -106,3 +106,3 @@ case 'z': -error = summary_string.SetCurrentValue(option_arg); +error = summary_string.SetCurrentValue(option_arg.str().c_str()); break; Please add llvm::StringRef version of SetCurrentValue to OptionValueString: https://reviews.llvm.org/D24847 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits