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

[Greg Clayton via lldb-commits]

clayborg added a comment.

Ok, so just add the boolean arg to Args::GetArgumentVector and we are good to go.


================
Comment at: source/Interpreter/Args.cpp:270-272
@@ -321,5 +269,5 @@
 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;
 }
----------------
Ok, sounds good, for a later CL then.

================
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);
----------------
zturner wrote:
> 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.
Remove the comment. Even in "main(int argc, const char **argv)" the "argv" is null terminated. So all unix variants expect the null termination. The correct solution is to add a boolean argument:

```
std::vector<const char *> Args::GetArgumentVector(bool null_terminate = true) const {
```

Then it is clear.

================
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;
----------------
zturner wrote:
> 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`.
Sounds good. I hate the fact that we have to do this. We might be able to play with the getopt_long globals to work around this and not have to add it, but as long as you took it into account I am fine.


https://reviews.llvm.org/D24952