[Lldb-commits] [PATCH] D25099: Refactor Args a different way

[Zachary Turner via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL283157: Refactor the Args class. (authored by zturner).

Changed prior to commit:
  https://reviews.llvm.org/D25099?vs=73335=73418#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D25099

Files:
  lldb/trunk/include/lldb/Interpreter/Args.h
  lldb/trunk/source/Core/Logging.cpp
  lldb/trunk/source/Core/StringList.cpp
  lldb/trunk/source/Interpreter/Args.cpp
  lldb/trunk/unittests/Interpreter/TestArgs.cpp

Index: lldb/trunk/unittests/Interpreter/TestArgs.cpp
===
--- lldb/trunk/unittests/Interpreter/TestArgs.cpp
+++ lldb/trunk/unittests/Interpreter/TestArgs.cpp
@@ -66,6 +66,109 @@
   EXPECT_STREQ(args.GetArgumentAtIndex(1), "second_arg");
 }
 
+TEST(ArgsTest, TestInsertArg) {
+  Args args;
+  args.AppendArgument("1");
+  args.AppendArgument("2");
+  args.AppendArgument("3");
+  args.InsertArgumentAtIndex(1, "1.5");
+  args.InsertArgumentAtIndex(4, "3.5");
+
+  ASSERT_EQ(5u, args.GetArgumentCount());
+  EXPECT_STREQ("1", args.GetArgumentAtIndex(0));
+  EXPECT_STREQ("1.5", args.GetArgumentAtIndex(1));
+  EXPECT_STREQ("2", args.GetArgumentAtIndex(2));
+  EXPECT_STREQ("3", args.GetArgumentAtIndex(3));
+  EXPECT_STREQ("3.5", args.GetArgumentAtIndex(4));
+}
+
+TEST(ArgsTest, TestArgv) {
+  Args args;
+  EXPECT_EQ(nullptr, args.GetArgumentVector());
+
+  args.AppendArgument("1");
+  EXPECT_NE(nullptr, args.GetArgumentVector()[0]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[1]);
+
+  args.AppendArgument("2");
+  EXPECT_NE(nullptr, args.GetArgumentVector()[0]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[1]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[2]);
+
+  args.AppendArgument("3");
+  EXPECT_NE(nullptr, args.GetArgumentVector()[0]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[1]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[2]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[3]);
+
+  args.InsertArgumentAtIndex(1, "1.5");
+  EXPECT_NE(nullptr, args.GetArgumentVector()[0]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[1]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[2]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[3]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[4]);
+
+  args.InsertArgumentAtIndex(4, "3.5");
+  EXPECT_NE(nullptr, args.GetArgumentVector()[0]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[1]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[2]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[3]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[4]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[5]);
+}
+
+TEST(ArgsTest, GetQuotedCommandString) {
+  Args args;
+  const char *str = "process launch -o stdout.txt -- \"a b c\"";
+  args.SetCommandString(str);
+
+  std::string stdstr;
+  ASSERT_TRUE(args.GetQuotedCommandString(stdstr));
+  EXPECT_EQ(str, stdstr);
+}
+
+TEST(ArgsTest, BareSingleQuote) {
+  Args args;
+  args.SetCommandString("a\\'b");
+  EXPECT_EQ(1u, args.GetArgumentCount());
+
+  EXPECT_STREQ("a'b", args.GetArgumentAtIndex(0));
+}
+
+TEST(ArgsTest, DoubleQuotedItem) {
+  Args args;
+  args.SetCommandString("\"a b c\"");
+  EXPECT_EQ(1u, args.GetArgumentCount());
+
+  EXPECT_STREQ("a b c", args.GetArgumentAtIndex(0));
+}
+
+TEST(ArgsTest, AppendArguments) {
+  Args args;
+  const char *argv[] = {"1", "2", nullptr};
+  const char *argv2[] = {"3", "4", nullptr};
+
+  args.AppendArguments(argv);
+  ASSERT_EQ(2u, args.GetArgumentCount());
+  EXPECT_STREQ("1", args.GetArgumentVector()[0]);
+  EXPECT_STREQ("2", args.GetArgumentVector()[1]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[2]);
+  EXPECT_STREQ("1", args.GetArgumentAtIndex(0));
+  EXPECT_STREQ("2", args.GetArgumentAtIndex(1));
+
+  args.AppendArguments(argv2);
+  ASSERT_EQ(4u, args.GetArgumentCount());
+  EXPECT_STREQ("1", args.GetArgumentVector()[0]);
+  EXPECT_STREQ("2", args.GetArgumentVector()[1]);
+  EXPECT_STREQ("3", args.GetArgumentVector()[2]);
+  EXPECT_STREQ("4", args.GetArgumentVector()[3]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[4]);
+  EXPECT_STREQ("1", args.GetArgumentAtIndex(0));
+  EXPECT_STREQ("2", args.GetArgumentAtIndex(1));
+  EXPECT_STREQ("3", args.GetArgumentAtIndex(2));
+  EXPECT_STREQ("4", args.GetArgumentAtIndex(3));
+}
+
 TEST(ArgsTest, StringToBoolean) {
   bool success = false;
   EXPECT_TRUE(Args::StringToBoolean(llvm::StringRef("true"), false, nullptr));
Index: lldb/trunk/source/Core/StringList.cpp
===
--- lldb/trunk/source/Core/StringList.cpp
+++ lldb/trunk/source/Core/StringList.cpp
@@ -103,34 +103,21 @@
 void StringList::Clear() { m_strings.clear(); }
 
 void StringList::LongestCommonPrefix(std::string _prefix) {
-  const size_t num_strings = m_strings.size();
-
-  if (num_strings == 0) {
-common_prefix.clear();
-  } else {
-common_prefix = m_strings.front();
-
-for (size_t idx = 1; idx < num_strings; ++idx) {
-  std::string 

Re: [Lldb-commits] [PATCH] D25099: Refactor Args a different way

[Zachary Turner via lldb-commits]

Thanks, I'll fix it up before submitting
On Mon, Oct 3, 2016 at 2:40 PM Jim Ingham  wrote:

> jingham added a comment.
>
> You messed up the meaning of one comment (noted inline).  Otherwise this
> looks fine to me too.
>
>
>
> > Args.cpp:97-98
> > +  // Argument can be split into multiple discontiguous pieces, for
> example:
> > +  //  "Hello " "World"
> > +  // this would result in a single argument "Hello World" (without the
> quotes)
> > +  // since the quotes would be removed and there is not space between
> the
>
> The example needs to be "Hello ""World" or it doesn't make sense.  "Hello
> " "World" with a space in between would be two arguments.
>
> https://reviews.llvm.org/D25099
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D25099: Refactor Args a different way

[Jim Ingham via lldb-commits]

jingham added a comment.

You messed up the meaning of one comment (noted inline).  Otherwise this looks 
fine to me too.



> Args.cpp:97-98
> +  // Argument can be split into multiple discontiguous pieces, for example:
> +  //  "Hello " "World"
> +  // this would result in a single argument "Hello World" (without the 
> quotes)
> +  // since the quotes would be removed and there is not space between the

The example needs to be "Hello ""World" or it doesn't make sense.  "Hello " 
"World" with a space in between would be two arguments.

https://reviews.llvm.org/D25099



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D25099: Refactor Args a different way

[Todd Fiala via lldb-commits]

tfiala accepted this revision.
tfiala added a comment.
This revision is now accepted and ready to land.

That works fine.

LGTM.


https://reviews.llvm.org/D25099



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D25099: Refactor Args a different way

[Todd Fiala via lldb-commits]

tfiala added a comment.

In https://reviews.llvm.org/D25099#559701, @zturner wrote:

> I know what this is.  It should be fixed in this patch, I guess I didn't have 
> the newest patch uploaded.


Okay, I'll give that a shot now.


https://reviews.llvm.org/D25099



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D25099: Refactor Args a different way

[Zachary Turner via lldb-commits]

zturner updated this revision to Diff 73335.
zturner added a comment.

I know what this is.  It should be fixed in this patch, I guess I didn't have 
the newest patch uploaded.


https://reviews.llvm.org/D25099

Files:
  include/lldb/Interpreter/Args.h
  source/Core/Logging.cpp
  source/Core/StringList.cpp
  source/Interpreter/Args.cpp
  unittests/Interpreter/TestArgs.cpp

Index: unittests/Interpreter/TestArgs.cpp
===
--- unittests/Interpreter/TestArgs.cpp
+++ unittests/Interpreter/TestArgs.cpp
@@ -66,6 +66,109 @@
   EXPECT_STREQ(args.GetArgumentAtIndex(1), "second_arg");
 }
 
+TEST(ArgsTest, TestInsertArg) {
+  Args args;
+  args.AppendArgument("1");
+  args.AppendArgument("2");
+  args.AppendArgument("3");
+  args.InsertArgumentAtIndex(1, "1.5");
+  args.InsertArgumentAtIndex(4, "3.5");
+
+  ASSERT_EQ(5u, args.GetArgumentCount());
+  EXPECT_STREQ("1", args.GetArgumentAtIndex(0));
+  EXPECT_STREQ("1.5", args.GetArgumentAtIndex(1));
+  EXPECT_STREQ("2", args.GetArgumentAtIndex(2));
+  EXPECT_STREQ("3", args.GetArgumentAtIndex(3));
+  EXPECT_STREQ("3.5", args.GetArgumentAtIndex(4));
+}
+
+TEST(ArgsTest, TestArgv) {
+  Args args;
+  EXPECT_EQ(nullptr, args.GetArgumentVector());
+
+  args.AppendArgument("1");
+  EXPECT_NE(nullptr, args.GetArgumentVector()[0]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[1]);
+
+  args.AppendArgument("2");
+  EXPECT_NE(nullptr, args.GetArgumentVector()[0]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[1]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[2]);
+
+  args.AppendArgument("3");
+  EXPECT_NE(nullptr, args.GetArgumentVector()[0]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[1]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[2]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[3]);
+
+  args.InsertArgumentAtIndex(1, "1.5");
+  EXPECT_NE(nullptr, args.GetArgumentVector()[0]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[1]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[2]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[3]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[4]);
+
+  args.InsertArgumentAtIndex(4, "3.5");
+  EXPECT_NE(nullptr, args.GetArgumentVector()[0]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[1]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[2]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[3]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[4]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[5]);
+}
+
+TEST(ArgsTest, GetQuotedCommandString) {
+  Args args;
+  const char *str = "process launch -o stdout.txt -- \"a b c\"";
+  args.SetCommandString(str);
+
+  std::string stdstr;
+  ASSERT_TRUE(args.GetQuotedCommandString(stdstr));
+  EXPECT_EQ(str, stdstr);
+}
+
+TEST(ArgsTest, BareSingleQuote) {
+  Args args;
+  args.SetCommandString("a\\'b");
+  EXPECT_EQ(1u, args.GetArgumentCount());
+
+  EXPECT_STREQ("a'b", args.GetArgumentAtIndex(0));
+}
+
+TEST(ArgsTest, DoubleQuotedItem) {
+  Args args;
+  args.SetCommandString("\"a b c\"");
+  EXPECT_EQ(1u, args.GetArgumentCount());
+
+  EXPECT_STREQ("a b c", args.GetArgumentAtIndex(0));
+}
+
+TEST(ArgsTest, AppendArguments) {
+  Args args;
+  const char *argv[] = {"1", "2", nullptr};
+  const char *argv2[] = {"3", "4", nullptr};
+
+  args.AppendArguments(argv);
+  ASSERT_EQ(2u, args.GetArgumentCount());
+  EXPECT_STREQ("1", args.GetArgumentVector()[0]);
+  EXPECT_STREQ("2", args.GetArgumentVector()[1]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[2]);
+  EXPECT_STREQ("1", args.GetArgumentAtIndex(0));
+  EXPECT_STREQ("2", args.GetArgumentAtIndex(1));
+
+  args.AppendArguments(argv2);
+  ASSERT_EQ(4u, args.GetArgumentCount());
+  EXPECT_STREQ("1", args.GetArgumentVector()[0]);
+  EXPECT_STREQ("2", args.GetArgumentVector()[1]);
+  EXPECT_STREQ("3", args.GetArgumentVector()[2]);
+  EXPECT_STREQ("4", args.GetArgumentVector()[3]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[4]);
+  EXPECT_STREQ("1", args.GetArgumentAtIndex(0));
+  EXPECT_STREQ("2", args.GetArgumentAtIndex(1));
+  EXPECT_STREQ("3", args.GetArgumentAtIndex(2));
+  EXPECT_STREQ("4", args.GetArgumentAtIndex(3));
+}
+
 TEST(ArgsTest, StringToBoolean) {
   bool success = false;
   EXPECT_TRUE(Args::StringToBoolean(llvm::StringRef("true"), false, nullptr));
Index: source/Interpreter/Args.cpp
===
--- source/Interpreter/Args.cpp
+++ source/Interpreter/Args.cpp
@@ -25,95 +25,12 @@
 #include "lldb/Target/StackFrame.h"
 #include "lldb/Target/Target.h"
 
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSwitch.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
-//--
-// Args constructor
-//--
-Args::Args(llvm::StringRef command) : m_args(), m_argv(), m_args_quote_char() {
-  SetCommandString(command);
-}
-

[Lldb-commits] [PATCH] D25099: Refactor Args a different way

[Todd Fiala via lldb-commits]

tfiala added a comment.

I will test this on macOS.  I will have the results this afternoon.


https://reviews.llvm.org/D25099



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D25099: Refactor Args a different way

[Todd Fiala via lldb-commits]

Yep I plan on doing that.

-Todd

> On Oct 3, 2016, at 10:29 AM, Zachary Turner  wrote:
> 
> He lgtm'ed my last patch, so I guess he's ok with the general concept.  
> Perhaps if someone could just run the test suite for me that would be good 
> enough.
> 
>> On Mon, Oct 3, 2016 at 10:25 AM Todd Fiala  wrote:
>> tfiala added a comment.
>> 
>> @zturner , Greg is out this week (and was last Friday as well).
>> 
>> I'll get somebody over here to review.
>> 
>> 
>> https://reviews.llvm.org/D25099
>> 
>> 
>> 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D25099: Refactor Args a different way

[Zachary Turner via lldb-commits]

He lgtm'ed my last patch, so I guess he's ok with the general concept.
Perhaps if someone could just run the test suite for me that would be good
enough.

On Mon, Oct 3, 2016 at 10:25 AM Todd Fiala  wrote:

> tfiala added a comment.
>
> @zturner , Greg is out this week (and was last Friday as well).
>
> I'll get somebody over here to review.
>
>
> https://reviews.llvm.org/D25099
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D25099: Refactor Args a different way

[Todd Fiala via lldb-commits]

tfiala added a comment.

@zturner , Greg is out this week (and was last Friday as well).

I'll get somebody over here to review.


https://reviews.llvm.org/D25099



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D25099: Refactor Args a different way

[Zachary Turner via lldb-commits]

zturner added a comment.

Hi Greg, might you have a chance to look at this today?  I've got a huge 
backlog of CLs to get in.  The rest probably won't require reviews, but this 
one is a precursor to everything else.


https://reviews.llvm.org/D25099



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D25099: Refactor Args a different way

[Zachary Turner via lldb-commits]

zturner added inline comments.


> labath wrote in Args.cpp:207
> I think we don't need to call Clear() here, as all memory owned by the object 
> will be correctly destroyed anyway.

Thanks for pointing that out.  Originally I wasn't using `unique_ptr` but 
rather `new[]` and `delete[]`.  After I changed to `unique_ptr` this 
became unnecessary and I forgot to remove it.  Thanks for pointing it out.

https://reviews.llvm.org/D25099



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D25099: Refactor Args a different way

[Pavel Labath via lldb-commits]

labath added a comment.

random drive-by. Otherwise, I like it.



> Args.cpp:207
> +//--
> +Args::~Args() { Clear(); }
> +

I think we don't need to call Clear() here, as all memory owned by the object 
will be correctly destroyed anyway.

https://reviews.llvm.org/D25099



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D25099: Refactor Args a different way

[Zachary Turner via lldb-commits]

zturner updated this revision to Diff 73095.

https://reviews.llvm.org/D25099

Files:
  include/lldb/Interpreter/Args.h
  source/Core/StringList.cpp
  source/Interpreter/Args.cpp
  unittests/Interpreter/TestArgs.cpp

Index: unittests/Interpreter/TestArgs.cpp
===
--- unittests/Interpreter/TestArgs.cpp
+++ unittests/Interpreter/TestArgs.cpp
@@ -66,6 +66,109 @@
   EXPECT_STREQ(args.GetArgumentAtIndex(1), "second_arg");
 }
 
+TEST(ArgsTest, TestInsertArg) {
+  Args args;
+  args.AppendArgument("1");
+  args.AppendArgument("2");
+  args.AppendArgument("3");
+  args.InsertArgumentAtIndex(1, "1.5");
+  args.InsertArgumentAtIndex(4, "3.5");
+
+  ASSERT_EQ(5u, args.GetArgumentCount());
+  EXPECT_STREQ("1", args.GetArgumentAtIndex(0));
+  EXPECT_STREQ("1.5", args.GetArgumentAtIndex(1));
+  EXPECT_STREQ("2", args.GetArgumentAtIndex(2));
+  EXPECT_STREQ("3", args.GetArgumentAtIndex(3));
+  EXPECT_STREQ("3.5", args.GetArgumentAtIndex(4));
+}
+
+TEST(ArgsTest, TestArgv) {
+  Args args;
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[0]);
+
+  args.AppendArgument("1");
+  EXPECT_NE(nullptr, args.GetArgumentVector()[0]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[1]);
+
+  args.AppendArgument("2");
+  EXPECT_NE(nullptr, args.GetArgumentVector()[0]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[1]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[2]);
+
+  args.AppendArgument("3");
+  EXPECT_NE(nullptr, args.GetArgumentVector()[0]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[1]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[2]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[3]);
+
+  args.InsertArgumentAtIndex(1, "1.5");
+  EXPECT_NE(nullptr, args.GetArgumentVector()[0]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[1]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[2]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[3]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[4]);
+
+  args.InsertArgumentAtIndex(4, "3.5");
+  EXPECT_NE(nullptr, args.GetArgumentVector()[0]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[1]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[2]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[3]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[4]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[5]);
+}
+
+TEST(ArgsTest, GetQuotedCommandString) {
+  Args args;
+  const char *str = "process launch -o stdout.txt -- \"a b c\"";
+  args.SetCommandString(str);
+
+  std::string stdstr;
+  ASSERT_TRUE(args.GetQuotedCommandString(stdstr));
+  EXPECT_EQ(str, stdstr);
+}
+
+TEST(ArgsTest, BareSingleQuote) {
+  Args args;
+  args.SetCommandString("a\\'b");
+  EXPECT_EQ(1u, args.GetArgumentCount());
+
+  EXPECT_STREQ("a'b", args.GetArgumentAtIndex(0));
+}
+
+TEST(ArgsTest, DoubleQuotedItem) {
+  Args args;
+  args.SetCommandString("\"a b c\"");
+  EXPECT_EQ(1u, args.GetArgumentCount());
+
+  EXPECT_STREQ("a b c", args.GetArgumentAtIndex(0));
+}
+
+TEST(ArgsTest, AppendArguments) {
+  Args args;
+  const char *argv[] = {"1", "2", nullptr};
+  const char *argv2[] = {"3", "4", nullptr};
+
+  args.AppendArguments(argv);
+  ASSERT_EQ(2u, args.GetArgumentCount());
+  EXPECT_STREQ("1", args.GetArgumentVector()[0]);
+  EXPECT_STREQ("2", args.GetArgumentVector()[1]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[2]);
+  EXPECT_STREQ("1", args.GetArgumentAtIndex(0));
+  EXPECT_STREQ("2", args.GetArgumentAtIndex(1));
+
+  args.AppendArguments(argv2);
+  ASSERT_EQ(4u, args.GetArgumentCount());
+  EXPECT_STREQ("1", args.GetArgumentVector()[0]);
+  EXPECT_STREQ("2", args.GetArgumentVector()[1]);
+  EXPECT_STREQ("3", args.GetArgumentVector()[2]);
+  EXPECT_STREQ("4", args.GetArgumentVector()[3]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[4]);
+  EXPECT_STREQ("1", args.GetArgumentAtIndex(0));
+  EXPECT_STREQ("2", args.GetArgumentAtIndex(1));
+  EXPECT_STREQ("3", args.GetArgumentAtIndex(2));
+  EXPECT_STREQ("4", args.GetArgumentAtIndex(3));
+}
+
 TEST(ArgsTest, StringToBoolean) {
   bool success = false;
   EXPECT_TRUE(Args::StringToBoolean(llvm::StringRef("true"), false, nullptr));
Index: source/Interpreter/Args.cpp
===
--- source/Interpreter/Args.cpp
+++ source/Interpreter/Args.cpp
@@ -25,95 +25,12 @@
 #include "lldb/Target/StackFrame.h"
 #include "lldb/Target/Target.h"
 
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSwitch.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
-//--
-// Args constructor
-//--
-Args::Args(llvm::StringRef command) : m_args(), m_argv(), m_args_quote_char() {
-  SetCommandString(command);
-}
-
-//--
-// We have to be very careful on the copy constructor of this class
-// to make sure we copy all of the string values, but we can't 

[Lldb-commits] [PATCH] D25099: Refactor Args a different way

[Zachary Turner via lldb-commits]

zturner updated this revision to Diff 73099.
zturner added a comment.

Renamed `EntryData` to `ArgEntry` and made it public.  This will be useful 
later for providing iterator support over the entries.


https://reviews.llvm.org/D25099

Files:
  include/lldb/Interpreter/Args.h
  source/Core/StringList.cpp
  source/Interpreter/Args.cpp
  unittests/Interpreter/TestArgs.cpp

Index: unittests/Interpreter/TestArgs.cpp
===
--- unittests/Interpreter/TestArgs.cpp
+++ unittests/Interpreter/TestArgs.cpp
@@ -66,6 +66,109 @@
   EXPECT_STREQ(args.GetArgumentAtIndex(1), "second_arg");
 }
 
+TEST(ArgsTest, TestInsertArg) {
+  Args args;
+  args.AppendArgument("1");
+  args.AppendArgument("2");
+  args.AppendArgument("3");
+  args.InsertArgumentAtIndex(1, "1.5");
+  args.InsertArgumentAtIndex(4, "3.5");
+
+  ASSERT_EQ(5u, args.GetArgumentCount());
+  EXPECT_STREQ("1", args.GetArgumentAtIndex(0));
+  EXPECT_STREQ("1.5", args.GetArgumentAtIndex(1));
+  EXPECT_STREQ("2", args.GetArgumentAtIndex(2));
+  EXPECT_STREQ("3", args.GetArgumentAtIndex(3));
+  EXPECT_STREQ("3.5", args.GetArgumentAtIndex(4));
+}
+
+TEST(ArgsTest, TestArgv) {
+  Args args;
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[0]);
+
+  args.AppendArgument("1");
+  EXPECT_NE(nullptr, args.GetArgumentVector()[0]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[1]);
+
+  args.AppendArgument("2");
+  EXPECT_NE(nullptr, args.GetArgumentVector()[0]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[1]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[2]);
+
+  args.AppendArgument("3");
+  EXPECT_NE(nullptr, args.GetArgumentVector()[0]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[1]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[2]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[3]);
+
+  args.InsertArgumentAtIndex(1, "1.5");
+  EXPECT_NE(nullptr, args.GetArgumentVector()[0]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[1]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[2]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[3]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[4]);
+
+  args.InsertArgumentAtIndex(4, "3.5");
+  EXPECT_NE(nullptr, args.GetArgumentVector()[0]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[1]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[2]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[3]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[4]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[5]);
+}
+
+TEST(ArgsTest, GetQuotedCommandString) {
+  Args args;
+  const char *str = "process launch -o stdout.txt -- \"a b c\"";
+  args.SetCommandString(str);
+
+  std::string stdstr;
+  ASSERT_TRUE(args.GetQuotedCommandString(stdstr));
+  EXPECT_EQ(str, stdstr);
+}
+
+TEST(ArgsTest, BareSingleQuote) {
+  Args args;
+  args.SetCommandString("a\\'b");
+  EXPECT_EQ(1u, args.GetArgumentCount());
+
+  EXPECT_STREQ("a'b", args.GetArgumentAtIndex(0));
+}
+
+TEST(ArgsTest, DoubleQuotedItem) {
+  Args args;
+  args.SetCommandString("\"a b c\"");
+  EXPECT_EQ(1u, args.GetArgumentCount());
+
+  EXPECT_STREQ("a b c", args.GetArgumentAtIndex(0));
+}
+
+TEST(ArgsTest, AppendArguments) {
+  Args args;
+  const char *argv[] = {"1", "2", nullptr};
+  const char *argv2[] = {"3", "4", nullptr};
+
+  args.AppendArguments(argv);
+  ASSERT_EQ(2u, args.GetArgumentCount());
+  EXPECT_STREQ("1", args.GetArgumentVector()[0]);
+  EXPECT_STREQ("2", args.GetArgumentVector()[1]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[2]);
+  EXPECT_STREQ("1", args.GetArgumentAtIndex(0));
+  EXPECT_STREQ("2", args.GetArgumentAtIndex(1));
+
+  args.AppendArguments(argv2);
+  ASSERT_EQ(4u, args.GetArgumentCount());
+  EXPECT_STREQ("1", args.GetArgumentVector()[0]);
+  EXPECT_STREQ("2", args.GetArgumentVector()[1]);
+  EXPECT_STREQ("3", args.GetArgumentVector()[2]);
+  EXPECT_STREQ("4", args.GetArgumentVector()[3]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[4]);
+  EXPECT_STREQ("1", args.GetArgumentAtIndex(0));
+  EXPECT_STREQ("2", args.GetArgumentAtIndex(1));
+  EXPECT_STREQ("3", args.GetArgumentAtIndex(2));
+  EXPECT_STREQ("4", args.GetArgumentAtIndex(3));
+}
+
 TEST(ArgsTest, StringToBoolean) {
   bool success = false;
   EXPECT_TRUE(Args::StringToBoolean(llvm::StringRef("true"), false, nullptr));
Index: source/Interpreter/Args.cpp
===
--- source/Interpreter/Args.cpp
+++ source/Interpreter/Args.cpp
@@ -25,95 +25,12 @@
 #include "lldb/Target/StackFrame.h"
 #include "lldb/Target/Target.h"
 
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSwitch.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
-//--
-// Args constructor
-//--
-Args::Args(llvm::StringRef command) : m_args(), m_argv(), m_args_quote_char() {
-  SetCommandString(command);
-}
-

[Lldb-commits] [PATCH] D25099: Refactor Args a different way

[Adrian McCarthy via lldb-commits]

amccarth added a comment.

Just a drive by.



> Args.h:449
>//--
>// Classes that inherit from Args can see and modify these
>//--

This comment is no longer true given the change from protected to private just 
above.

> Args.cpp:96
> +ParseSingleArgument(llvm::StringRef command) {
> +  // Argument can be split into multiple discontiguous pieces, // for 
> example:
>//  "Hello ""World"

Minor formatting glitch with the `\\` in the comment.

> Args.cpp:192
> +//--
> +// We have to be very careful on the copy constructor of this class
> +// to make sure we copy all of the string values, but we can't copy the

This says "copy constructor" but it seems to be documenting the copy assignment 
operator.

> Args.cpp:195
> +// rhs.m_argv into m_argv since it will point to the "const char *" c
> +// strings in rhs.m_args. We need to copy the string list and update our
> +// own m_argv appropriately.

You got right of m_args, so maybe this whole comment needs a rewrite.

> Args.cpp:282
> +
> +  // Now m_argv might be out of date with m_args, so we need to fix that.
> +  // This happens because getopt_long_only may permute the order of the

`m_args` is gone.

https://reviews.llvm.org/D25099



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D25099: Refactor Args a different way

[Zachary Turner via lldb-commits]

zturner created this revision.
zturner added a reviewer: clayborg.
zturner added subscribers: lldb-commits, LLDB.

In https://reviews.llvm.org/D24952 I tried to refactor the `Args` class to get 
rid of `m_argv`.  The original motivation for this is that I wanted a way to 
implement `GetArgumentAtIndex()` so as to return a `StringRef`.  I could have 
just returned a `StringRef` from the `m_argv` array, but it seems wasteful to 
calculate the length every time when we were already storing a `std::string`.   
Unfortunately the `std::string` was in a `std::list` (required so that it would 
not be invalidated when inserting items into the arg list) so there was no 
random access.  So I wanted to see if I could do better, and that was the 
motivation for the original patch.

However, after fixing up all the places in the code to use the "new" `Args`, I 
wasn't completely satisfied.  I like that the old code could just give you an 
`argv` pointer that you can pass to a system API.  So I started over and ended 
up with this.  Quick overview of changes:

1. Instead of using a `std::list` to own the memory, we use a 
`std::vector>`.  This provides random access, which is 
something `list` couldn't provide, and it also provides non-const access to the 
underlying buffer, which is something that `std::string` couldn't provide, 
leading to a lot of `const_cast` which are no longer necessary.
2. Each entry's quote char, backing memory (i.e. `std::unique_ptr`), 
and `StringRef` with precomputed length are stored together in a single entry.  
This guarantees by design that the various arrays' lengths do not need to stay 
in sync with each other, because there is only one array.
3. Some longstanding undefined behavior is documented and left for someone else 
to fix.
4. Asserts are added to the code to document the pre-conditions.  I know we are 
allergic to asserts, but I want to emphasize that none of these asserts have 
anything to do with the input parameters.  The asserts are ALWAYS on internal 
class state.

With this change it should finally be easy to change `GetArgumentAtIndex()` to 
return a `StringRef`, because it can just return `m_entries[idx].ref`.


https://reviews.llvm.org/D25099

Files:
  include/lldb/Interpreter/Args.h
  source/Core/StringList.cpp
  source/Interpreter/Args.cpp
  unittests/Interpreter/TestArgs.cpp

Index: unittests/Interpreter/TestArgs.cpp
===
--- unittests/Interpreter/TestArgs.cpp
+++ unittests/Interpreter/TestArgs.cpp
@@ -66,6 +66,109 @@
   EXPECT_STREQ(args.GetArgumentAtIndex(1), "second_arg");
 }
 
+TEST(ArgsTest, TestInsertArg) {
+  Args args;
+  args.AppendArgument("1");
+  args.AppendArgument("2");
+  args.AppendArgument("3");
+  args.InsertArgumentAtIndex(1, "1.5");
+  args.InsertArgumentAtIndex(4, "3.5");
+
+  ASSERT_EQ(5u, args.GetArgumentCount());
+  EXPECT_STREQ("1", args.GetArgumentAtIndex(0));
+  EXPECT_STREQ("1.5", args.GetArgumentAtIndex(1));
+  EXPECT_STREQ("2", args.GetArgumentAtIndex(2));
+  EXPECT_STREQ("3", args.GetArgumentAtIndex(3));
+  EXPECT_STREQ("3.5", args.GetArgumentAtIndex(4));
+}
+
+TEST(ArgsTest, TestArgv) {
+  Args args;
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[0]);
+
+  args.AppendArgument("1");
+  EXPECT_NE(nullptr, args.GetArgumentVector()[0]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[1]);
+
+  args.AppendArgument("2");
+  EXPECT_NE(nullptr, args.GetArgumentVector()[0]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[1]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[2]);
+
+  args.AppendArgument("3");
+  EXPECT_NE(nullptr, args.GetArgumentVector()[0]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[1]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[2]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[3]);
+
+  args.InsertArgumentAtIndex(1, "1.5");
+  EXPECT_NE(nullptr, args.GetArgumentVector()[0]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[1]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[2]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[3]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[4]);
+
+  args.InsertArgumentAtIndex(4, "3.5");
+  EXPECT_NE(nullptr, args.GetArgumentVector()[0]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[1]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[2]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[3]);
+  EXPECT_NE(nullptr, args.GetArgumentVector()[4]);
+  EXPECT_EQ(nullptr, args.GetArgumentVector()[5]);
+}
+
+TEST(ArgsTest, GetQuotedCommandString) {
+  Args args;
+  const char *str = "process launch -o stdout.txt -- \"a b c\"";
+  args.SetCommandString(str);
+
+  std::string stdstr;
+  ASSERT_TRUE(args.GetQuotedCommandString(stdstr));
+  EXPECT_EQ(str, stdstr);
+}
+
+TEST(ArgsTest, BareSingleQuote) {
+  Args args;
+  args.SetCommandString("a\\'b");
+  EXPECT_EQ(1u, args.GetArgumentCount());
+
+  EXPECT_STREQ("a'b", args.GetArgumentAtIndex(0));
+}
+
+TEST(ArgsTest, DoubleQuotedItem) {
+  Args args;
+