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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
