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

[Adrian McCarthy via lldb-commits]

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