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