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

[Zachary Turner via lldb-commits]

zturner added inline comments.

================
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:
> > 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.
> Even in main though, the null is not counted in the argc, right?  So as written it will conform to the semantics of main.  `argc` is the number of non-null pointers, but the null is still there anyway.  If you add the boolean argument and pass true, it will give you back something whose `argc` (i.e. `result.size()`) is one too high.
BTW, `llvm::SmallString` uses this same trick to return a null terminated string without modifying the size.  Look at the implementation of `SmallString::c_str()` in `SmallString.h`


https://reviews.llvm.org/D24952