[Lldb-commits] [PATCH] D24952: Remove Args::m_argv

[Greg Clayton via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

See inlined comments.


================
Comment at: source/Interpreter/Args.cpp:264-265
@@ -315,3 +263,4 @@
 size_t Args::GetArgumentCount() const {
-  if (m_argv.empty())
+  if (m_args.empty())
     return 0;
+  return m_args.size();
----------------
Remove this if, it is no longer needed. Just return m_args.size()

================
Comment at: source/Interpreter/Args.cpp:269-272
@@ -320,6 +268,6 @@
 
 const char *Args::GetArgumentAtIndex(size_t idx) const {
-  if (idx < m_argv.size())
-    return m_argv[idx];
+  if (idx < m_args.size())
+    return m_args[idx].c_str();
   return nullptr;
 }
----------------
Convert this over to return StringRef?

================
Comment at: source/Interpreter/Args.cpp:286-288
@@ -338,5 +285,5 @@
 
-const char **Args::GetConstArgumentVector() const {
-  if (!m_argv.empty())
-    return const_cast<const char **>(&m_argv[0]);
-  return nullptr;
+  // This is kind of gross, but it ensures that if someone passes args.data() to
+  // a function which expects an array terminated with a null entry, it will
+  // work.
+  result.push_back(nullptr);
----------------
Remove this comment. The whole point off this function is to get an argument vector that is NULL terminated. Feel free to rename the function as desired to indicate this, but that is the whole point of this function. Feel free to document it if needed as well.

================
Comment at: source/Interpreter/Args.cpp:327
@@ -381,11 +326,3 @@
                                             char quote_char) {
-  // Since we are using a std::list to hold onto the copied C string and
-  // we don't have direct access to the elements, we have to iterate to
-  // find the value.
-  arg_sstr_collection::iterator pos, end = m_args.end();
-  size_t i = idx;
-  for (pos = m_args.begin(); i > 0 && pos != end; ++pos)
-    --i;
-
-  pos = m_args.insert(pos, arg_str);
+  m_args.insert(m_args.begin() + idx, arg_str);
 
----------------
Is there a conversion operator that converts StringRef to std::string somewhere? How is this working? Iterators being detected?

================
Comment at: source/Interpreter/Args.cpp:373-376
@@ -460,21 +372,6 @@
 
-void Args::SetArguments(const char **argv) {
-  // m_argv will be rebuilt in UpdateArgvFromArgs() below, so there is
-  // no need to clear it here.
-  m_args.clear();
-  m_args_quote_char.clear();
-
-  if (argv) {
-    // First copy each string
-    for (size_t i = 0; argv[i]; ++i) {
-      m_args.push_back(argv[i]);
-      if ((argv[i][0] == '\'') || (argv[i][0] == '"') || (argv[i][0] == '`'))
-        m_args_quote_char.push_back(argv[i][0]);
-      else
-        m_args_quote_char.push_back('\0');
-    }
-  }
-
-  UpdateArgvFromArgs();
+void Args::SetArguments(const Args &other) {
+  m_args = other.m_args;
+  m_args_quote_char = other.m_args_quote_char;
 }
 
----------------
Should we change this to an assignment operator? If so, rename "other" to "rhs".

================
Comment at: source/Interpreter/CommandInterpreter.cpp:2049-2050
@@ -2048,5 +2048,4 @@
 
     cmd_args.Clear();
-    cmd_args.SetArguments(new_args.GetArgumentCount(),
-                          new_args.GetConstArgumentVector());
+    cmd_args.SetArguments(new_args);
   } else {
----------------
If we do the operator = above, then:

```
cmd_args = new_args;
```

================
Comment at: source/Interpreter/CommandInterpreter.cpp:2059-2060
@@ -2059,5 +2058,4 @@
     if (wants_raw_input) {
       cmd_args.Clear();
-      cmd_args.SetArguments(new_args.GetArgumentCount(),
-                            new_args.GetConstArgumentVector());
+      cmd_args.SetArguments(new_args);
     }
----------------
If we do the operator = above, then:

```
cmd_args = new_args;
```

================
Comment at: source/Interpreter/CommandObject.cpp:119
@@ -118,3 +118,2 @@
     // so we need to push a dummy value into position zero.
-    args.Unshift(llvm::StringRef("dummy_string"));
     const bool require_validation = true;
----------------
Why was this removed?

================
Comment at: source/Interpreter/OptionValueArgs.cpp:33
@@ -32,3 +32,3 @@
   else
-    args.SetArguments(argv.size(), &argv[0]);
+    args.SetArguments(argv);
   return args.GetArgumentCount();
----------------
args = argv;

================
Comment at: source/Interpreter/OptionValueArray.cpp:151
@@ -150,3 +150,3 @@
   else
-    args.SetArguments(argv.size(), &argv[0]);
+    args.SetArguments(argv);
   return args.GetArgumentCount();
----------------
args = argv;

================
Comment at: source/Target/ProcessInfo.cpp:94
@@ -93,3 +93,3 @@
                                bool first_arg_is_executable) {
-  m_arguments.SetArguments(argv);
+  m_arguments.SetArguments(Args::ArgvToArrayRef(argv));
 
----------------
What not have a SetArguments that takes a "const char **" and does this internally? It is no less safe and makes the API more usable


https://reviews.llvm.org/D24952