zturner created this revision.
zturner added reviewers: jingham, beanz, clayborg, labath.
zturner added a subscriber: lldb-commits.
The purpose of this patch is to demonstrate my proposed new API to the `Args`
class. There are a couple of things I'm trying to demonstrate here:
1. range-based for loops by iterating directly over the `Args` instance itself.
This should be the preferred way to iterate over arguments and should always
be used unless index-based iteration is required.
2. range-based for loops by iterating over `args.entries()`. This should be
used when you only want to iterate a subset of the arguments, like if you want
to skip the first one (e.g. `for (const auto &entry :
args.entries().drop_front())`. This prevents many types of bugs, such as those
posted to the list recently in which we repeatedly access `args[0]` instead of
`args[i]`.
3. Index-based iteration through the use of `operator[]`. Should only be used
when you need to do look ahead or look-behind, or a command takes positional
arguments. Prefer range-based for otherwise. This method supersedes
`GetArgumentAtIndex` and the long term plan is to delete `GetArgumentAtIndex`
after all call-sites have been converted.
4. Direct access to `StringRef` through the use of `entry.ref`. Should always
be used unless you're printing or passing to a C-api.
5. Direct access to the quote char through the use of `entry.quote`.
Supersedes `GetArgumentQuoteCharAtIndex`.
6. Access to a null-terminated c-string. Should only be used when printf'ing
the value or passing to a C-api (which, btw, should almost never be done for
any kind of string manipulation operation, as `StringRef` provides everything
you need). Long term I plan to delete this accessor, the only thing blocking
this is the heavy use of printf-style formatting (which I have a solution for
in mind). In any case, it should not be relied on for any kind of algorithmic
access to the underlying string.
7. A couple of simple examples of how this proposed new API can trivially
reduce string copies and improve efficiency.
All dependent calls have already been updated to take `StringRef`, so the
"phase 2", so to speak, will be to begin eliminating calls to
`GetArgumentAtIndex` using the methods described above. Once they are all
gone, the plan is to delete `GetArgumentAtIndex` and `GetQuoteCharAtIndex`.
In the more distant future, if I can make my `Printf` alternative a reality, I
plan to delete c-style string access entirely by removing the `c_str()` method
from `ArgEntry`.
https://reviews.llvm.org/D26883
Files:
include/lldb/Interpreter/Args.h
source/Breakpoint/BreakpointIDList.cpp
source/Commands/CommandObjectCommands.cpp
source/Commands/CommandObjectFrame.cpp
source/Commands/CommandObjectProcess.cpp
source/Commands/CommandObjectSettings.cpp
source/Interpreter/CommandInterpreter.cpp
source/Interpreter/OptionValueDictionary.cpp
Index: source/Interpreter/OptionValueDictionary.cpp
===================================================================
--- source/Interpreter/OptionValueDictionary.cpp
+++ source/Interpreter/OptionValueDictionary.cpp
@@ -101,73 +101,74 @@
case eVarSetOperationAppend:
case eVarSetOperationReplace:
case eVarSetOperationAssign:
- if (argc > 0) {
- for (size_t i = 0; i < argc; ++i) {
- llvm::StringRef key_and_value(args.GetArgumentAtIndex(i));
- if (!key_and_value.empty()) {
- if (key_and_value.find('=') == llvm::StringRef::npos) {
- error.SetErrorString(
- "assign operation takes one or more key=value arguments");
- return error;
- }
+ if (argc == 0) {
+ error.SetErrorString(
+ "assign operation takes one or more key=value arguments");
+ return error;
+ }
+ for (const auto &entry : args) {
+ if (entry.ref.empty()) {
+ error.SetErrorString("empty argument");
+ return error;
+ }
+ if (!entry.ref.contains('=')) {
+ error.SetErrorString(
+ "assign operation takes one or more key=value arguments");
+ return error;
+ }
+
+ llvm::StringRef key, value;
+ std::tie(key, value) = entry.ref.split('=');
+ bool key_valid = false;
+ if (key.empty()) {
+ error.SetErrorString("empty dictionary key");
+ return error;
+ }
- std::pair<llvm::StringRef, llvm::StringRef> kvp(
- key_and_value.split('='));
- llvm::StringRef key = kvp.first;
- bool key_valid = false;
- if (!key.empty()) {
- if (key.front() == '[') {
- // Key name starts with '[', so the key value must be in single or
- // double quotes like:
- // ['<key>']
- // ["<key>"]
- if ((key.size() > 2) && (key.back() == ']')) {
- // Strip leading '[' and trailing ']'
- key = key.substr(1, key.size() - 2);
- const char quote_char = key.front();
- if ((quote_char == '\'') || (quote_char == '"')) {
- if ((key.size() > 2) && (key.back() == quote_char)) {
- // Strip the quotes
- key = key.substr(1, key.size() - 2);
- key_valid = true;
- }
- } else {
- // square brackets, no quotes
- key_valid = true;
- }
- }
- } else {
- // No square brackets or quotes
+ if (key.front() == '[') {
+ // Key name starts with '[', so the key value must be in single or
+ // double quotes like:
+ // ['<key>']
+ // ["<key>"]
+ if ((key.size() > 2) && (key.back() == ']')) {
+ // Strip leading '[' and trailing ']'
+ key = key.substr(1, key.size() - 2);
+ const char quote_char = key.front();
+ if ((quote_char == '\'') || (quote_char == '"')) {
+ if ((key.size() > 2) && (key.back() == quote_char)) {
+ // Strip the quotes
+ key = key.substr(1, key.size() - 2);
key_valid = true;
}
- }
- if (!key_valid) {
- error.SetErrorStringWithFormat(
- "invalid key \"%s\", the key must be a bare string or "
- "surrounded by brackets with optional quotes: [<key>] or "
- "['<key>'] or [\"<key>\"]",
- kvp.first.str().c_str());
- return error;
- }
-
- lldb::OptionValueSP value_sp(CreateValueFromCStringForTypeMask(
- kvp.second.data(), m_type_mask, error));
- if (value_sp) {
- if (error.Fail())
- return error;
- m_value_was_set = true;
- SetValueForKey(ConstString(key), value_sp, true);
} else {
- error.SetErrorString("dictionaries that can contain multiple types "
- "must subclass OptionValueArray");
+ // square brackets, no quotes
+ key_valid = true;
}
- } else {
- error.SetErrorString("empty argument");
}
+ } else {
+ // No square brackets or quotes
+ key_valid = true;
+ }
+ if (!key_valid) {
+ error.SetErrorStringWithFormat(
+ "invalid key \"%s\", the key must be a bare string or "
+ "surrounded by brackets with optional quotes: [<key>] or "
+ "['<key>'] or [\"<key>\"]",
+ key.str().c_str());
+ return error;
+ }
+
+ lldb::OptionValueSP value_sp(CreateValueFromCStringForTypeMask(
+ value.str().c_str(), m_type_mask, error));
+ if (value_sp) {
+ if (error.Fail())
+ return error;
+ m_value_was_set = true;
+ SetValueForKey(ConstString(key), value_sp, true);
+ } else {
+ error.SetErrorString("dictionaries that can contain multiple types "
+ "must subclass OptionValueArray");
}
- } else {
- error.SetErrorString(
- "assign operation takes one or more key=value arguments");
}
break;
Index: source/Interpreter/CommandInterpreter.cpp
===================================================================
--- source/Interpreter/CommandInterpreter.cpp
+++ source/Interpreter/CommandInterpreter.cpp
@@ -1859,9 +1859,8 @@
// put an empty string in element 0.
std::string command_partial_str;
if (cursor_index >= 0)
- command_partial_str.assign(parsed_line.GetArgumentAtIndex(cursor_index),
- parsed_line.GetArgumentAtIndex(cursor_index) +
- cursor_char_position);
+ command_partial_str =
+ parsed_line[cursor_index].ref.take_front(cursor_char_position);
std::string common_prefix;
matches.LongestCommonPrefix(common_prefix);
@@ -1872,7 +1871,7 @@
// Only do this if the completer told us this was a complete word,
// however...
if (num_command_matches == 1 && word_complete) {
- char quote_char = parsed_line.GetArgumentQuoteCharAtIndex(cursor_index);
+ char quote_char = parsed_line[cursor_index].quote;
common_prefix =
Args::EscapeLLDBCommandArgument(common_prefix, quote_char);
if (quote_char != '\0')
Index: source/Commands/CommandObjectSettings.cpp
===================================================================
--- source/Commands/CommandObjectSettings.cpp
+++ source/Commands/CommandObjectSettings.cpp
@@ -295,13 +295,9 @@
const size_t argc = args.GetArgumentCount();
if (!args.empty()) {
- // TODO: Convert this to StringRef based enumeration. Requires converting
- // DumpPropertyValue first.
- for (size_t i = 0; i < argc; ++i) {
- const char *property_path = args.GetArgumentAtIndex(i);
-
+ for (const auto &arg : args) {
Error error(m_interpreter.GetDebugger().DumpPropertyValue(
- &m_exe_ctx, result.GetOutputStream(), property_path,
+ &m_exe_ctx, result.GetOutputStream(), arg.ref,
OptionValue::eDumpGroupValue));
if (error.Success()) {
result.GetOutputStream().EOL();
Index: source/Commands/CommandObjectProcess.cpp
===================================================================
--- source/Commands/CommandObjectProcess.cpp
+++ source/Commands/CommandObjectProcess.cpp
@@ -1558,9 +1558,8 @@
int num_signals_set = 0;
if (num_args > 0) {
- for (size_t i = 0; i < num_args; ++i) {
- int32_t signo = signals_sp->GetSignalNumberFromName(
- signal_args.GetArgumentAtIndex(i));
+ for (const auto &arg : signal_args) {
+ int32_t signo = signals_sp->GetSignalNumberFromName(arg.c_str());
if (signo != LLDB_INVALID_SIGNAL_NUMBER) {
// Casting the actions as bools here should be okay, because
// VerifyCommandOptionValue guarantees
@@ -1576,7 +1575,7 @@
++num_signals_set;
} else {
result.AppendErrorWithFormat("Invalid signal name '%s'\n",
- signal_args.GetArgumentAtIndex(i));
+ arg.c_str());
}
}
} else {
Index: source/Commands/CommandObjectFrame.cpp
===================================================================
--- source/Commands/CommandObjectFrame.cpp
+++ source/Commands/CommandObjectFrame.cpp
@@ -473,12 +473,12 @@
bool &word_complete,
StringList &matches) override {
// Arguments are the standard source file completer.
- std::string completion_str(input.GetArgumentAtIndex(cursor_index));
- completion_str.erase(cursor_char_position);
+ auto completion_str = input[cursor_index].ref;
+ completion_str = completion_str.take_front(cursor_char_position);
CommandCompletions::InvokeCommonCompletionCallbacks(
GetCommandInterpreter(), CommandCompletions::eVariablePathCompletion,
- completion_str.c_str(), match_start_point, max_return_elements, nullptr,
+ completion_str, match_start_point, max_return_elements, nullptr,
word_complete, matches);
return matches.GetSize();
}
Index: source/Commands/CommandObjectCommands.cpp
===================================================================
--- source/Commands/CommandObjectCommands.cpp
+++ source/Commands/CommandObjectCommands.cpp
@@ -1476,12 +1476,12 @@
int match_start_point, int max_return_elements,
bool &word_complete,
StringList &matches) override {
- std::string completion_str(input.GetArgumentAtIndex(cursor_index));
- completion_str.erase(cursor_char_position);
+ llvm::StringRef completion_str = input[cursor_index].ref;
+ completion_str = completion_str.take_front(cursor_char_position);
CommandCompletions::InvokeCommonCompletionCallbacks(
GetCommandInterpreter(), CommandCompletions::eDiskFileCompletion,
- completion_str.c_str(), match_start_point, max_return_elements, nullptr,
+ completion_str, match_start_point, max_return_elements, nullptr,
word_complete, matches);
return matches.GetSize();
}
Index: source/Breakpoint/BreakpointIDList.cpp
===================================================================
--- source/Breakpoint/BreakpointIDList.cpp
+++ source/Breakpoint/BreakpointIDList.cpp
@@ -122,13 +122,12 @@
llvm::StringRef range_from;
llvm::StringRef range_to;
llvm::StringRef current_arg;
- const size_t num_old_args = old_args.GetArgumentCount();
std::set<std::string> names_found;
- for (size_t i = 0; i < num_old_args; ++i) {
+ for (size_t i = 0; i < old_args.size(); ++i) {
bool is_range = false;
- current_arg = old_args.GetArgumentAtIndex(i);
+ current_arg = old_args[i].ref;
if (!allow_locations && current_arg.contains('.')) {
result.AppendErrorWithFormat(
"Breakpoint locations not allowed, saw location: %s.",
@@ -152,19 +151,17 @@
return;
} else
names_found.insert(current_arg);
- } else if ((i + 2 < num_old_args) &&
- BreakpointID::IsRangeIdentifier(
- old_args.GetArgumentAtIndex(i + 1)) &&
+ } else if ((i + 2 < old_args.size()) &&
+ BreakpointID::IsRangeIdentifier(old_args[i + 1].ref) &&
BreakpointID::IsValidIDExpression(current_arg) &&
- BreakpointID::IsValidIDExpression(
- old_args.GetArgumentAtIndex(i + 2))) {
+ BreakpointID::IsValidIDExpression(old_args[i + 2].ref)) {
range_from = current_arg;
- range_to = old_args.GetArgumentAtIndex(i + 2);
+ range_to = old_args[i + 2].ref;
is_range = true;
i = i + 2;
} else {
// See if user has specified id.*
- llvm::StringRef tmp_str = old_args.GetArgumentAtIndex(i);
+ llvm::StringRef tmp_str = old_args[i].ref;
size_t pos = tmp_str.find('.');
if (pos != llvm::StringRef::npos) {
llvm::StringRef bp_id_str = tmp_str.substr(0, pos);
Index: include/lldb/Interpreter/Args.h
===================================================================
--- include/lldb/Interpreter/Args.h
+++ include/lldb/Interpreter/Args.h
@@ -150,9 +150,14 @@
const char *GetArgumentAtIndex(size_t idx) const;
llvm::ArrayRef<ArgEntry> entries() const { return m_entries; }
-
char GetArgumentQuoteCharAtIndex(size_t idx) const;
+ auto begin() const { return m_entries.begin(); }
+ auto end() const { return m_entries.end(); }
+
+ size_t size() const { return GetArgumentCount(); }
+ const ArgEntry &operator[](size_t n) const { return m_entries[n]; }
+
//------------------------------------------------------------------
/// Gets the argument vector.
///
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
