amccarth added a subscriber: amccarth.
================
Comment at: include/lldb/Interpreter/Args.h:154
@@ -167,3 +153,3 @@
//------------------------------------------------------------------
- const char **GetConstArgumentVector() const;
+ void GetArgumentVector(std::vector<const char *> &args) const;
----------------
Perhaps this should actually return the vector instead of void, and rely on the
return value optimization or, at worst, vector move. It would be easier to use
and it would let you initialize variables as you declare them, (which means you
could use it in constructor intializers and you could declare the vector as
const). It also would eliminate the ambiguity of what happens if the `args`
already contains some values (e.g., the caller might expect it to append rather
than replace).
================
Comment at: source/API/SBCommandInterpreter.cpp:119
@@ +118,3 @@
+ std::vector<const char *> argv;
+ command.GetArgumentVector(argv);
+ bool ret = m_backend->DoExecute(debugger_sb, &argv[0], sb_return);
----------------
Here's an example where, if GetArgumentVector actually returned the result, you
could simplify the delcaration as:
const auto argv = command.GetArgumentVector();
================
Comment at: source/Host/common/FileSpec.cpp:251
@@ +250,3 @@
+
+ for (auto &name : name_list) {
+ matches.AppendString(name);
----------------
Should `name` be `const`? As in:
for (const auto &name : name_list) { ... }
================
Comment at: source/Interpreter/Args.cpp:46
@@ -44,3 +45,3 @@
// strings in rhs.m_args. We need to copy the string list and update our
// own m_argv appropriately.
//----------------------------------------------------------------------
----------------
I think your change eliminated the need for this comment. If not, it at least
needs an update.
================
Comment at: source/Interpreter/Args.cpp:275
@@ +274,3 @@
+ // new_argv.
+ // So make a copy and then swap it in.
+ std::vector<std::string> new_args(new_argv.begin(), new_argv.end());
----------------
clang-format didn't do a great job wrapping the long lines in this comment.
Can you fix it up?
================
Comment at: source/Interpreter/Args.cpp:427
@@ -512,1 +426,3 @@
+ GetArgumentVector(args);
+ args.insert(args.begin(), "dummy");
while (1) {
----------------
This is an example where the ambiguity of whether GetArgumentVector appends or
not could mislead the caller into doing the wrong this. This looks plausible
(and more efficient):
std::vector<const char *> args;
args.push_back("dummy");
GetArgumentVector(args);
If GetArgumentVector returned the vector, then this ambiguity wouldn't exist.
https://reviews.llvm.org/D24952
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
