Demonstrate proposed new Args API

[zturner]

Summary

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.

[labath]

I don't know how deep do you want this refactor to be, but there is one issue I would like us to consider, if only to decide it is out of scope of this change. I am talking about the quote_char thingy. The main problem for me is that I don't think it's possible to sanely define the meaning of that field. According to POSIX quoting rules (which our command line more-or-less follows) a single argument can be quoted in a great many ways, using various combinations of quote characters. For example, these are all valid ways to represent the argument asdf in a POSIX shell:

asdf
"asdf"
'asdf'
a"sd"f
"as"df
"as""df"
"as"'df'
"a"s'd'"f"
... (you get my point)

I don't think there is a self-consistent way to define what the quote_char field will be for each of these options. Moreover, I don't see why one would ever need to use that field. It can only encourage someone to try to "quote" the argument by doing quote_char+value+quote_char, which is absolutely wrong if you ever want that result to be machine parsable.(*) For proper quoting I think we should just have a free-standing std::string quote_for_posix_shell(llvm::StringRef) function (and maybe quote_for_windows_cmd, and whatever else quoting scheme we need), and then the user can decide which one to use based on who is going to be consuming it. Then we can just kill the quote field. The only thing is... I have no idea how much work that will be (but I am ready to chip in to make it happen).

So, yea, if we decide not to do that, then I think the interface looks great. Otherwise, I think we can design a slightly simpler (and more consistent) one.

(*) Bonus question: Try to start an executable under lldb, so that in enters main() with argc=2 and argv[1]="'" I.e., as if it had been started this way via bash:

$ /bin/cat \'

[labath]

Assuming we do that, what interface do you think would be simpler? We still
need easy access to both a StringRef and a c_str(), since StringRef::data
is not guaranteed to be null terminated, so the entry thing is still nice.

I was assuming (possibly incorrectly, I did not look at that code much) that the main user of the null-terminated string version was execve(2), which needs an entire list of strings, not just a single item, in which case we could have the iteration simply be over StringRefs. That said, it probably does not matter now, as judging by your comments, you're looking for incremental changes, not one grand final design (btw, I didn't mean to suggest that this should all be done in one patch). In that case, this looks fine as far as I am concerned. We can always revise this later, and it doesn't look like it will require any major rewrite, just a bit of syntax-twiddling. You can then read my comments as "the direction I would like to move us in".

One idea might be to have the entry contain 2 StringRefs. str and
quoted_str. This way you never get access to the underlying quote char,
just the full arg, either quoted or unquoted (although doing this would
still be better done orthogonally to this patch)

I don't think this is a good idea, as I don't see a need to be able to access the original quoted string this way. Also, when you construct the args vector programmatically, you would have to invent a quoted representation without knowing if it will ever be used. I'd prefer to have a standalone quote function instead, as then it can be used in other contexts as well (separate algorithms from data).

[jingham]

In D26883#600683, @labath wrote:

I don't know how deep do you want this refactor to be, but there is one issue I would like us to consider, if only to decide it is out of scope of this change. I am talking about the quote_char thingy. The main problem for me is that I don't think it's possible to sanely define the meaning of that field. According to POSIX quoting rules (which our command line more-or-less follows) a single argument can be quoted in a great many ways, using various combinations of quote characters. For example, these are all valid ways to represent the argument asdf in a POSIX shell:

asdf
"asdf"
'asdf'
a"sd"f
"as"df
"as""df"
"as"'df'
"a"s'd'"f"
... (you get my point)

I don't think there is a self-consistent way to define what the quote_char field will be for each of these options. Moreover, I don't see why one would ever need to use that field. It can only encourage someone to try to "quote" the argument by doing quote_char+value+quote_char, which is absolutely wrong if you ever want that result to be machine parsable.(*) For proper quoting I think we should just have a free-standing std::string quote_for_posix_shell(llvm::StringRef) function (and maybe quote_for_windows_cmd, and whatever else quoting scheme we need), and then the user can decide which one to use based on who is going to be consuming it. Then we can just kill the quote field. The only thing is... I have no idea how much work that will be (but I am ready to chip in to make it happen).

So, yea, if we decide not to do that, then I think the interface looks great. Otherwise, I think we can design a slightly simpler (and more consistent) one.

(*) Bonus question: Try to start an executable under lldb, so that in enters main() with argc=2 and argv[1]="'" I.e., as if it had been started this way via bash:

$ /bin/cat \'

The outermost quote character is syntactically significant in the lldb command language. If you say:

memory read -c count_var 0x123345

then lldb evaluates the expression in the backticks, replacing the argument with the result of the expression. So you can't get rid of the quote character altogether. The other use is in completion to figure out what an unterminated quoted string should be completed with.

Since only the outermost quoting character is important, I think the problem is more tractable than your examples would suggest.

[zturner]

Instead of storing the quote char, I think we could just store two StringRefs. str and quoted_str, where str = quoted_str.trim(quote_char);, and if there is no quote char, then they are equal. I'd rather do that in a followup since this ArgEntry class has already been here for some time and is kind of orthogonal to what's going on here, but it seems like a sane approach, as then you don't have to worry about anyone reconstructing the quoted version from the unquoted version, it's just already there.

[labath]

In D26883#601638, @jingham wrote:

The outermost quote character is syntactically significant in the lldb command language. If you say:

memory read -c count_var 0x123345

then lldb evaluates the expression in the backticks, replacing the argument with the result of the expression. So you can't get rid of the quote character altogether. The other use is in completion to figure out what an unterminated quoted string should be completed with.

Since only the outermost quoting character is important, I think the problem is more tractable than your examples would suggest.

I didn't mention backticks, as (to use bash parlor) command substitution and word splitting are two separate passes. backticks can be embedded into a single word, which also contains other kinds of quote characters, so storing the quote character alongside the word-split argument is not going to help you implement command substitution. LLDB actually handles this mostly correctly now (notice that in each case the input gets parsed as a single word):

(lldb) `1+1`
error: '2' is not a valid command.
error: Unrecognized command '2'.
(lldb) a`1+1`a
error: 'a2a' is not a valid command.
error: Unrecognized command 'a2a'.
(lldb) `1+1`a`2+2`
error: '2a4' is not a valid command.
error: Unrecognized command '2a4'.
(lldb) "a a"`1+1`"b a"
error: unknown command shorthand suffix: ' a'
error: Unrecognized command 'a a2b a'.

The reason this works is because the command substitution is done in a different place in the code, so I still hold my view that the Args class does not need to differentiate between the different quote characters as it is just a holder for a word-split input.

That said, none of this should affect this patch. If the quote char field has been there until now, it can stay a little longer.

lldb/trunk/include/lldb/Interpreter/Args.h

public:
   ///
   /// @return
   ///     The NULL terminated C string argument pointer if \a idx is a
   ///     valid argument index, NULL otherwise.
   //------------------------------------------------------------------
   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.
   ///
   /// The value returned by this function can be used by any function
   /// that takes and vector. The return value is just like \a argv
   /// in the standard C entry point function:
   ///     \code
   ///         int main (int argc, const char **argv);

lldb/trunk/source/Breakpoint/BreakpointIDList.cpp

 
 void BreakpointIDList::FindAndReplaceIDRanges(Args &old_args, Target *target,
                                               bool allow_locations,
                                               CommandReturnObject &result,
                                               Args &new_args) {
   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.",
           current_arg.str().c_str());
       new_args.Clear();
       return;
     }
 
     llvm::StringRef range_expr;
     Error error;
 
     std::tie(range_from, range_to) =
         BreakpointIDList::SplitIDRangeExpression(current_arg);
     if (!range_from.empty() && !range_to.empty()) {
       is_range = true;
     } else if (BreakpointID::StringIsBreakpointName(current_arg, error)) {
       if (!error.Success()) {
         new_args.Clear();
         result.AppendError(error.AsCString());
         result.SetStatus(eReturnStatusFailed);
         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);
         if (BreakpointID::IsValidIDExpression(bp_id_str) &&
             tmp_str[pos + 1] == '*' && tmp_str.size() == (pos + 2)) {
 
           BreakpointSP breakpoint_sp;
           auto bp_id = BreakpointID::ParseCanonicalReference(bp_id_str);

lldb/trunk/source/Commands/CommandObjectCommands.cpp

public:
   ~CommandObjectCommandsScriptImport() override = default;
 
   int HandleArgumentCompletion(Args &input, int &cursor_index,
                                int &cursor_char_position,
                                OptionElementVector &opt_element_vector,
                                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();
   }
 
   Options *GetOptions() override { return &m_options; }
 
 protected:
   class CommandOptions : public Options {

lldb/trunk/source/Commands/CommandObjectFrame.cpp

public:
 
   int HandleArgumentCompletion(Args &input, int &cursor_index,
                                int &cursor_char_position,
                                OptionElementVector &opt_element_vector,
                                int match_start_point, int max_return_elements,
                                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();
   }
 
 protected:
   llvm::StringRef GetScopeString(VariableSP var_sp) {
     if (!var_sp)
       return llvm::StringRef::withNullAsEmpty(nullptr);

lldb/trunk/source/Commands/CommandObjectProcess.cpp

if (!m_options.pass.empty() &&
       return false;
     }
 
     size_t num_args = signal_args.GetArgumentCount();
     UnixSignalsSP signals_sp = process_sp->GetUnixSignals();
     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
           // the value is either 0 or 1.
           if (stop_action != -1)
             signals_sp->SetShouldStop(signo, stop_action);
           if (pass_action != -1) {
             bool suppress = !pass_action;
             signals_sp->SetShouldSuppress(signo, suppress);
           }
           if (notify_action != -1)
             signals_sp->SetShouldNotify(signo, notify_action);
           ++num_signals_set;
         } else {
           result.AppendErrorWithFormat("Invalid signal name '%s'\n",
-                                       signal_args.GetArgumentAtIndex(i));
+                                       arg.c_str());
         }
       }
     } else {
       // No signal specified, if any command options were specified, update ALL
       // signals.
       if ((notify_action != -1) || (stop_action != -1) || (pass_action != -1)) {
         if (m_interpreter.Confirm(
                 "Do you really want to update all the signals?", false)) {

lldb/trunk/source/Commands/CommandObjectSettings.cpp

CommandCompletions::InvokeCommonCompletionCallbacks(
         word_complete, matches);
     return matches.GetSize();
   }
 
 protected:
   bool DoExecute(Args &args, CommandReturnObject &result) override {
     result.SetStatus(eReturnStatusSuccessFinishResult);
 
-    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();
         } else {
           result.AppendError(error.AsCString());
           result.SetStatus(eReturnStatusFailed);
         }
       }

lldb/trunk/source/Interpreter/CommandInterpreter.cpp

if (num_args == 0) {
     // If we got an empty string, insert nothing.
     matches.InsertStringAtIndex(0, "");
   } else {
     // Now figure out if there is a common substring, and if so put that in
     // element 0, otherwise
     // 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);
     const size_t partial_name_len = command_partial_str.size();
     common_prefix.erase(0, partial_name_len);
 
     // If we matched a unique single command, add a space...
     // 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')
         common_prefix.push_back(quote_char);
       common_prefix.push_back(' ');
     }
     matches.InsertStringAtIndex(0, common_prefix.c_str());
   }

lldb/trunk/source/Interpreter/OptionValueDictionary.cpp

Error OptionValueDictionary::SetArgs(const Args &args, VarSetOperationType op) {
   switch (op) {
   case eVarSetOperationClear:
     Clear();
     break;
 
   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('=')) {
 
-          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
-              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;
-          }
+      llvm::StringRef key, value;
+      std::tie(key, value) = entry.ref.split('=');
+      if (key.empty()) {
+        error.SetErrorString("empty dictionary key");
+        return error;
+      }
+
+            key.str().c_str());
 
-          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");
-          }
-        } else {
-          error.SetErrorString("empty argument");
-        }
-      }
-    } else {
-      error.SetErrorString(
-          "assign operation takes one or more key=value arguments");
+          value.str().c_str(), m_type_mask, error));
     }
     break;
 
   case eVarSetOperationRemove:
     if (argc > 0) {
       for (size_t i = 0; i < argc; ++i) {
         ConstString key(args.GetArgumentAtIndex(i));
         if (!DeleteValueForKey(key)) {