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

[Zachary Turner via lldb-commits]

zturner added inline comments.

================
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;
 }
----------------
clayborg wrote:
> Convert this over to return StringRef?
Yes, that is the next item on my list.  But this function is used EVERYWHERE, so that will be a very large CL and should be done orthogonally.

================
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);
----------------
clayborg wrote:
> 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.
There are two ways we use these argument vectors.  One is in a call to a function which takes an argc and an argv.  In that case the null terminator is unnecessary because you're specifying the argc explicitly.  Another is in a call to a function which you do not specify an argc, and in tht case it requires it to be null terminated.

The point of this function isn't "return me something that's null terminated", it's "return me something I can use like an argv".  That doesn't require null termination.  And in fact, if you look at the callsites I fixed up, not a single one depends on the null termination.  Everyone just needs to know the size.  And for that you can call `result.size()`.  If you add the null terminator, then you have to write `result.size()-1`, which is unnecessarily confusing.  I believe it provides the most logical behavior as it is currently written.

================
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);
 
----------------
clayborg wrote:
> Is there a conversion operator that converts StringRef to std::string somewhere? How is this working? Iterators being detected?
Yes, StringRef has an `operator std::string()`.

================
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;
 }
 
----------------
clayborg wrote:
> Should we change this to an assignment operator? If so, rename "other" to "rhs".
I thought about it, but it seems like a minor stylistic issue.  I think it could go either way.  I'm fine adding it or not.

================
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;
----------------
clayborg wrote:
> Why was this removed?
Because it's a little weird to do it at this high of a level.  This requires internal knowledge of the workings of `Args::ParseOptions()`, which itself calls `OptionParser::Parse()`, It's strange for a function this high up to require such low-level knowledge of the workings of a function.  So instead, I moved this behavior into `Args::ParseOptions`.  If you check that function, you will see that it injects this argument into the beginning.

This is nice because now it never modifies the internal state of the `Args` itself, but only the copy that gets passed to `getopt_long_only`.

================
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));
 
----------------
clayborg wrote:
> 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
I would eventually like to get rid of all of these `const char*`'s.  For example, this function's signature could itself be changed to take an `ArrayRef<const char *>`.  Then we can keep raising this conversion higher and higher up.  I actually //want// to make it more awkward to use unsafe types like `const char **`.  So by adding the `const char **` overload to `Args`, it doesn't encourage people to consider the alternative `ArrayRef`.  On the other hand, this is similar to `StringRef`, in that once you have the `ArrayRef<const char *>` through all levels, you will almost never need this function, and calls like this will disappear as the `ArrayRefs` are being passed all the way down.


https://reviews.llvm.org/D24952