Modify ParseArgs to return the InputArgList by value - there's no need for dynamic...

The one caller that does anything other than keep this variable on the
stack is the single use of DerivedArgList in Clang, which is a bit more
interesting but can probably be cleaned up/simplified a bit further
(have DerivedArgList take ownership of the InputArgList rather than
needing to reference its Args indirectly) which I'll try to after this.

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@240345 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/llvm/Option/ArgList.h b/include/llvm/Option/ArgList.h
index 15d82d6da359d0dada3887bb39ba8735e53dd139..e29cf24661a6cc83990971efb5e381f91ff33331 100644 (file)
--- a/include/llvm/Option/ArgList.h
+++ b/include/llvm/Option/ArgList.h
@@ -92,10 +92,6 @@ public:
 /// check for the presence of Arg instances for a particular Option
 /// and to iterate over groups of arguments.
 class ArgList {
-private:
-  ArgList(const ArgList &) = delete;
-  void operator=(const ArgList &) = delete;
-
 public:
   typedef SmallVector<Arg*, 16> arglist_type;
   typedef arglist_type::iterator iterator;
@@ -108,9 +104,21 @@ private:
   arglist_type Args;
 
 protected:
-  // Default ctor provided explicitly as it is not provided implicitly due to
-  // the presence of the (deleted) copy ctor above.
+  // Make the default special members protected so they won't be used to slice
+  // derived objects, but can still be used by derived objects to implement
+  // their own special members.
   ArgList() = default;
+  // Explicit move operations to ensure the container is cleared post-move
+  // otherwise it could lead to a double-delete in the case of moving of an
+  // InputArgList which deletes the contents of the container. If we could fix
+  // up the ownership here (delegate storage/ownership to the derived class so
+  // it can be a container of unique_ptr) this would be simpler.
+  ArgList(ArgList &&RHS) : Args(std::move(RHS.Args)) { RHS.Args.clear(); }
+  ArgList &operator=(ArgList &&RHS) {
+    Args = std::move(RHS.Args);
+    RHS.Args.clear();
+    return *this;
+  }
   // Protect the dtor to ensure this type is never destroyed polymorphically.
   ~ArgList() = default;
 
@@ -319,6 +327,19 @@ private:
 
 public:
   InputArgList(const char* const *ArgBegin, const char* const *ArgEnd);
+  // Default move operations implemented for the convenience of MSVC. Nothing
+  // special here.
+  InputArgList(InputArgList &&RHS)
+      : ArgList(std::move(RHS)), ArgStrings(std::move(RHS.ArgStrings)),
+        SynthesizedStrings(std::move(RHS.SynthesizedStrings)),
+        NumInputArgStrings(RHS.NumInputArgStrings) {}
+  InputArgList &operator=(InputArgList &&RHS) {
+    ArgList::operator=(std::move(RHS));
+    ArgStrings = std::move(RHS.ArgStrings);
+    SynthesizedStrings = std::move(RHS.SynthesizedStrings);
+    NumInputArgStrings = RHS.NumInputArgStrings;
+    return *this;
+  }
   ~InputArgList();
 
   const char *getArgString(unsigned Index) const override {

diff --git a/include/llvm/Option/OptTable.h b/include/llvm/Option/OptTable.h
index 62bfb0852f87168e5364aa2bf72284a149d3c318..96f51cf3317d63e5495558b3ef543373f58dd766 100644 (file)
--- a/include/llvm/Option/OptTable.h
+++ b/include/llvm/Option/OptTable.h
@@ -151,10 +151,9 @@ public:
   /// is the default and means exclude nothing.
   /// \return An InputArgList; on error this will contain all the options
   /// which could be parsed.
-  InputArgList *ParseArgs(ArrayRef<const char *> Args,
-                          unsigned &MissingArgIndex, unsigned &MissingArgCount,
-                          unsigned FlagsToInclude = 0,
-                          unsigned FlagsToExclude = 0) const;
+  InputArgList ParseArgs(ArrayRef<const char *> Args, unsigned &MissingArgIndex,
+                         unsigned &MissingArgCount, unsigned FlagsToInclude = 0,
+                         unsigned FlagsToExclude = 0) const;
 
   /// \brief Render the help text for an option table.
   ///

diff --git a/lib/LibDriver/LibDriver.cpp b/lib/LibDriver/LibDriver.cpp
index e441fe84c39b673fdf891f87d17444a9ba3f32b9..9f2b12f75b8b2c25e07bc789cf65c514d809ef0b 100644 (file)
--- a/lib/LibDriver/LibDriver.cpp
+++ b/lib/LibDriver/LibDriver.cpp
@@ -113,27 +113,27 @@ int llvm::libDriverMain(llvm::ArrayRef<const char*> ArgsArr) {
   LibOptTable Table;
   unsigned MissingIndex;
   unsigned MissingCount;
-  std::unique_ptr<llvm::opt::InputArgList> Args(
-      Table.ParseArgs(ArgsArr.slice(1), MissingIndex, MissingCount));
+  llvm::opt::InputArgList Args =
+      Table.ParseArgs(ArgsArr.slice(1), MissingIndex, MissingCount);
   if (MissingCount) {
     llvm::errs() << "missing arg value for \""
-                 << Args->getArgString(MissingIndex)
-                 << "\", expected " << MissingCount
+                 << Args.getArgString(MissingIndex) << "\", expected "
+                 << MissingCount
                  << (MissingCount == 1 ? " argument.\n" : " arguments.\n");
     return 1;
   }
-  for (auto *Arg : Args->filtered(OPT_UNKNOWN))
+  for (auto *Arg : Args.filtered(OPT_UNKNOWN))
     llvm::errs() << "ignoring unknown argument: " << Arg->getSpelling() << "\n";
 
-  if (Args->filtered_begin(OPT_INPUT) == Args->filtered_end()) {
+  if (Args.filtered_begin(OPT_INPUT) == Args.filtered_end()) {
     llvm::errs() << "no input files.\n";
     return 1;
   }
 
-  std::vector<StringRef> SearchPaths = getSearchPaths(Args.get(), Saver);
+  std::vector<StringRef> SearchPaths = getSearchPaths(&Args, Saver);
 
   std::vector<llvm::NewArchiveIterator> Members;
-  for (auto *Arg : Args->filtered(OPT_INPUT)) {
+  for (auto *Arg : Args.filtered(OPT_INPUT)) {
     Optional<std::string> Path = findInputFile(Arg->getValue(), SearchPaths);
     if (!Path.hasValue()) {
       llvm::errs() << Arg->getValue() << ": no such file or directory\n";
@@ -143,8 +143,8 @@ int llvm::libDriverMain(llvm::ArrayRef<const char*> ArgsArr) {
                          llvm::sys::path::filename(Arg->getValue()));
   }
 
-  std::pair<StringRef, std::error_code> Result = llvm::writeArchive(
-      getOutputPath(Args.get()), Members, /*WriteSymtab=*/true);
+  std::pair<StringRef, std::error_code> Result =
+      llvm::writeArchive(getOutputPath(&Args), Members, /*WriteSymtab=*/true);
   if (Result.second) {
     if (Result.first.empty())
       Result.first = ArgsArr[0];

diff --git a/lib/Option/OptTable.cpp b/lib/Option/OptTable.cpp
index cec1717454f4a72a8b88092a7001c2cab56a0cbf..2160b15c1cf05d76c131080d3ac4b3e8ee4da444 100644 (file)
--- a/lib/Option/OptTable.cpp
+++ b/lib/Option/OptTable.cpp
@@ -247,12 +247,12 @@ Arg *OptTable::ParseOneArg(const ArgList &Args, unsigned &Index,
   return new Arg(getOption(TheUnknownOptionID), Str, Index++, Str);
 }
 
-InputArgList *OptTable::ParseArgs(ArrayRef<const char *> ArgArr,
-                                  unsigned &MissingArgIndex,
-                                  unsigned &MissingArgCount,
-                                  unsigned FlagsToInclude,
-                                  unsigned FlagsToExclude) const {
-  InputArgList *Args = new InputArgList(ArgArr.begin(), ArgArr.end());
+InputArgList OptTable::ParseArgs(ArrayRef<const char *> ArgArr,
+                                 unsigned &MissingArgIndex,
+                                 unsigned &MissingArgCount,
+                                 unsigned FlagsToInclude,
+                                 unsigned FlagsToExclude) const {
+  InputArgList Args(ArgArr.begin(), ArgArr.end());
 
   // FIXME: Handle '@' args (or at least error on them).
 
@@ -260,19 +260,19 @@ InputArgList *OptTable::ParseArgs(ArrayRef<const char *> ArgArr,
   unsigned Index = 0, End = ArgArr.size();
   while (Index < End) {
     // Ingore nullptrs, they are response file's EOL markers
-    if (Args->getArgString(Index) == nullptr) {
+    if (Args.getArgString(Index) == nullptr) {
       ++Index;
       continue;
     }
     // Ignore empty arguments (other things may still take them as arguments).
-    StringRef Str = Args->getArgString(Index);
+    StringRef Str = Args.getArgString(Index);
     if (Str == "") {
       ++Index;
       continue;
     }
 
     unsigned Prev = Index;
-    Arg *A = ParseOneArg(*Args, Index, FlagsToInclude, FlagsToExclude);
+    Arg *A = ParseOneArg(Args, Index, FlagsToInclude, FlagsToExclude);
     assert(Index > Prev && "Parser failed to consume argument.");
 
     // Check for missing argument error.
@@ -284,10 +284,10 @@ InputArgList *OptTable::ParseArgs(ArrayRef<const char *> ArgArr,
       break;
     }
 
-    Args->append(A);
+    Args.append(A);
   }
 
-  return Args;
+  return std::move(Args);
 }
 
 static std::string getOptionHelpName(const OptTable &Opts, OptSpecifier Id) {

diff --git a/unittests/Option/OptionParsingTest.cpp b/unittests/Option/OptionParsingTest.cpp
index 671cf6de0cb0b21dc976ad5f6251b06b4bbc7981..55cf8a95f35e7712a66a67afc139b11286f3656c 100644 (file)
--- a/unittests/Option/OptionParsingTest.cpp
+++ b/unittests/Option/OptionParsingTest.cpp
@@ -67,26 +67,26 @@ const char *Args[] = {
 TEST(Option, OptionParsing) {
   TestOptTable T;
   unsigned MAI, MAC;
-  std::unique_ptr<InputArgList> AL(T.ParseArgs(Args, MAI, MAC));
+  InputArgList AL = T.ParseArgs(Args, MAI, MAC);
 
   // Check they all exist.
-  EXPECT_TRUE(AL->hasArg(OPT_A));
-  EXPECT_TRUE(AL->hasArg(OPT_B));
-  EXPECT_TRUE(AL->hasArg(OPT_C));
-  EXPECT_TRUE(AL->hasArg(OPT_D));
-  EXPECT_TRUE(AL->hasArg(OPT_E));
-  EXPECT_TRUE(AL->hasArg(OPT_F));
-  EXPECT_TRUE(AL->hasArg(OPT_G));
+  EXPECT_TRUE(AL.hasArg(OPT_A));
+  EXPECT_TRUE(AL.hasArg(OPT_B));
+  EXPECT_TRUE(AL.hasArg(OPT_C));
+  EXPECT_TRUE(AL.hasArg(OPT_D));
+  EXPECT_TRUE(AL.hasArg(OPT_E));
+  EXPECT_TRUE(AL.hasArg(OPT_F));
+  EXPECT_TRUE(AL.hasArg(OPT_G));
 
   // Check the values.
-  EXPECT_EQ(AL->getLastArgValue(OPT_B), "hi");
-  EXPECT_EQ(AL->getLastArgValue(OPT_C), "bye");
-  EXPECT_EQ(AL->getLastArgValue(OPT_D), "adena");
-  std::vector<std::string> Es = AL->getAllArgValues(OPT_E);
+  EXPECT_EQ(AL.getLastArgValue(OPT_B), "hi");
+  EXPECT_EQ(AL.getLastArgValue(OPT_C), "bye");
+  EXPECT_EQ(AL.getLastArgValue(OPT_D), "adena");
+  std::vector<std::string> Es = AL.getAllArgValues(OPT_E);
   EXPECT_EQ(Es[0], "apple");
   EXPECT_EQ(Es[1], "bloom");
-  EXPECT_EQ(AL->getLastArgValue(OPT_F), "42");
-  std::vector<std::string> Gs = AL->getAllArgValues(OPT_G);
+  EXPECT_EQ(AL.getLastArgValue(OPT_F), "42");
+  std::vector<std::string> Gs = AL.getAllArgValues(OPT_G);
   EXPECT_EQ(Gs[0], "chuu");
   EXPECT_EQ(Gs[1], "2");
 
@@ -97,11 +97,11 @@ TEST(Option, OptionParsing) {
   EXPECT_NE(Help.find("-A"), std::string::npos);
 
   // Test aliases.
-  arg_iterator Cs = AL->filtered_begin(OPT_C);
-  ASSERT_NE(Cs, AL->filtered_end());
+  arg_iterator Cs = AL.filtered_begin(OPT_C);
+  ASSERT_NE(Cs, AL.filtered_end());
   EXPECT_EQ(StringRef((*Cs)->getValue()), "desu");
   ArgStringList ASL;
-  (*Cs)->render(*AL, ASL);
+  (*Cs)->render(AL, ASL);
   ASSERT_EQ(ASL.size(), 2u);
   EXPECT_EQ(StringRef(ASL[0]), "-C");
   EXPECT_EQ(StringRef(ASL[1]), "desu");
@@ -110,30 +110,29 @@ TEST(Option, OptionParsing) {
 TEST(Option, ParseWithFlagExclusions) {
   TestOptTable T;
   unsigned MAI, MAC;
-  std::unique_ptr<InputArgList> AL;
 
   // Exclude flag3 to avoid parsing as OPT_SLASH_C.
-  AL.reset(T.ParseArgs(Args, MAI, MAC,
-                       /*FlagsToInclude=*/0,
-                       /*FlagsToExclude=*/OptFlag3));
-  EXPECT_TRUE(AL->hasArg(OPT_A));
-  EXPECT_TRUE(AL->hasArg(OPT_C));
-  EXPECT_FALSE(AL->hasArg(OPT_SLASH_C));
+  InputArgList AL = T.ParseArgs(Args, MAI, MAC,
+                                /*FlagsToInclude=*/0,
+                                /*FlagsToExclude=*/OptFlag3);
+  EXPECT_TRUE(AL.hasArg(OPT_A));
+  EXPECT_TRUE(AL.hasArg(OPT_C));
+  EXPECT_FALSE(AL.hasArg(OPT_SLASH_C));
 
   // Exclude flag1 to avoid parsing as OPT_C.
-  AL.reset(T.ParseArgs(Args, MAI, MAC,
-                       /*FlagsToInclude=*/0,
-                       /*FlagsToExclude=*/OptFlag1));
-  EXPECT_TRUE(AL->hasArg(OPT_B));
-  EXPECT_FALSE(AL->hasArg(OPT_C));
-  EXPECT_TRUE(AL->hasArg(OPT_SLASH_C));
+  AL = T.ParseArgs(Args, MAI, MAC,
+                   /*FlagsToInclude=*/0,
+                   /*FlagsToExclude=*/OptFlag1);
+  EXPECT_TRUE(AL.hasArg(OPT_B));
+  EXPECT_FALSE(AL.hasArg(OPT_C));
+  EXPECT_TRUE(AL.hasArg(OPT_SLASH_C));
 
   const char *NewArgs[] = { "/C", "foo", "--C=bar" };
-  AL.reset(T.ParseArgs(NewArgs, MAI, MAC));
-  EXPECT_TRUE(AL->hasArg(OPT_SLASH_C));
-  EXPECT_TRUE(AL->hasArg(OPT_C));
-  EXPECT_EQ(AL->getLastArgValue(OPT_SLASH_C), "foo");
-  EXPECT_EQ(AL->getLastArgValue(OPT_C), "bar");
+  AL = T.ParseArgs(NewArgs, MAI, MAC);
+  EXPECT_TRUE(AL.hasArg(OPT_SLASH_C));
+  EXPECT_TRUE(AL.hasArg(OPT_C));
+  EXPECT_EQ(AL.getLastArgValue(OPT_SLASH_C), "foo");
+  EXPECT_EQ(AL.getLastArgValue(OPT_C), "bar");
 }
 
 TEST(Option, ParseAliasInGroup) {
@@ -141,8 +140,8 @@ TEST(Option, ParseAliasInGroup) {
   unsigned MAI, MAC;
 
   const char *MyArgs[] = { "-I" };
-  std::unique_ptr<InputArgList> AL(T.ParseArgs(MyArgs, MAI, MAC));
-  EXPECT_TRUE(AL->hasArg(OPT_H));
+  InputArgList AL = T.ParseArgs(MyArgs, MAI, MAC);
+  EXPECT_TRUE(AL.hasArg(OPT_H));
 }
 
 TEST(Option, AliasArgs) {
@@ -150,10 +149,10 @@ TEST(Option, AliasArgs) {
   unsigned MAI, MAC;
 
   const char *MyArgs[] = { "-J", "-Joo" };
-  std::unique_ptr<InputArgList> AL(T.ParseArgs(MyArgs, MAI, MAC));
-  EXPECT_TRUE(AL->hasArg(OPT_B));
-  EXPECT_EQ(AL->getAllArgValues(OPT_B)[0], "foo");
-  EXPECT_EQ(AL->getAllArgValues(OPT_B)[1], "bar");
+  InputArgList AL = T.ParseArgs(MyArgs, MAI, MAC);
+  EXPECT_TRUE(AL.hasArg(OPT_B));
+  EXPECT_EQ(AL.getAllArgValues(OPT_B)[0], "foo");
+  EXPECT_EQ(AL.getAllArgValues(OPT_B)[1], "bar");
 }
 
 TEST(Option, IgnoreCase) {
@@ -161,9 +160,9 @@ TEST(Option, IgnoreCase) {
   unsigned MAI, MAC;
 
   const char *MyArgs[] = { "-a", "-joo" };
-  std::unique_ptr<InputArgList> AL(T.ParseArgs(MyArgs, MAI, MAC));
-  EXPECT_TRUE(AL->hasArg(OPT_A));
-  EXPECT_TRUE(AL->hasArg(OPT_B));
+  InputArgList AL = T.ParseArgs(MyArgs, MAI, MAC);
+  EXPECT_TRUE(AL.hasArg(OPT_A));
+  EXPECT_TRUE(AL.hasArg(OPT_B));
 }
 
 TEST(Option, DoNotIgnoreCase) {
@@ -171,9 +170,9 @@ TEST(Option, DoNotIgnoreCase) {
   unsigned MAI, MAC;
 
   const char *MyArgs[] = { "-a", "-joo" };
-  std::unique_ptr<InputArgList> AL(T.ParseArgs(MyArgs, MAI, MAC));
-  EXPECT_FALSE(AL->hasArg(OPT_A));
-  EXPECT_FALSE(AL->hasArg(OPT_B));
+  InputArgList AL = T.ParseArgs(MyArgs, MAI, MAC);
+  EXPECT_FALSE(AL.hasArg(OPT_A));
+  EXPECT_FALSE(AL.hasArg(OPT_B));
 }
 
 TEST(Option, SlurpEmpty) {
@@ -181,10 +180,10 @@ TEST(Option, SlurpEmpty) {
   unsigned MAI, MAC;
 
   const char *MyArgs[] = { "-A", "-slurp" };
-  std::unique_ptr<InputArgList> AL(T.ParseArgs(MyArgs, MAI, MAC));
-  EXPECT_TRUE(AL->hasArg(OPT_A));
-  EXPECT_TRUE(AL->hasArg(OPT_Slurp));
-  EXPECT_EQ(AL->getAllArgValues(OPT_Slurp).size(), 0U);
+  InputArgList AL = T.ParseArgs(MyArgs, MAI, MAC);
+  EXPECT_TRUE(AL.hasArg(OPT_A));
+  EXPECT_TRUE(AL.hasArg(OPT_Slurp));
+  EXPECT_EQ(AL.getAllArgValues(OPT_Slurp).size(), 0U);
 }
 
 TEST(Option, Slurp) {
@@ -192,15 +191,15 @@ TEST(Option, Slurp) {
   unsigned MAI, MAC;
 
   const char *MyArgs[] = { "-A", "-slurp", "-B", "--", "foo" };
-  std::unique_ptr<InputArgList> AL(T.ParseArgs(MyArgs, MAI, MAC));
-  EXPECT_EQ(AL->size(), 2U);
-  EXPECT_TRUE(AL->hasArg(OPT_A));
-  EXPECT_FALSE(AL->hasArg(OPT_B));
-  EXPECT_TRUE(AL->hasArg(OPT_Slurp));
-  EXPECT_EQ(AL->getAllArgValues(OPT_Slurp).size(), 3U);
-  EXPECT_EQ(AL->getAllArgValues(OPT_Slurp)[0], "-B");
-  EXPECT_EQ(AL->getAllArgValues(OPT_Slurp)[1], "--");
-  EXPECT_EQ(AL->getAllArgValues(OPT_Slurp)[2], "foo");
+  InputArgList AL = T.ParseArgs(MyArgs, MAI, MAC);
+  EXPECT_EQ(AL.size(), 2U);
+  EXPECT_TRUE(AL.hasArg(OPT_A));
+  EXPECT_FALSE(AL.hasArg(OPT_B));
+  EXPECT_TRUE(AL.hasArg(OPT_Slurp));
+  EXPECT_EQ(AL.getAllArgValues(OPT_Slurp).size(), 3U);
+  EXPECT_EQ(AL.getAllArgValues(OPT_Slurp)[0], "-B");
+  EXPECT_EQ(AL.getAllArgValues(OPT_Slurp)[1], "--");
+  EXPECT_EQ(AL.getAllArgValues(OPT_Slurp)[2], "foo");
 }
 
 TEST(Option, FlagAliasToJoined) {
@@ -209,9 +208,9 @@ TEST(Option, FlagAliasToJoined) {
 
   // Check that a flag alias provides an empty argument to a joined option.
   const char *MyArgs[] = { "-K" };
-  std::unique_ptr<InputArgList> AL(T.ParseArgs(MyArgs, MAI, MAC));
-  EXPECT_EQ(AL->size(), 1U);
-  EXPECT_TRUE(AL->hasArg(OPT_B));
-  EXPECT_EQ(AL->getAllArgValues(OPT_B).size(), 1U);
-  EXPECT_EQ(AL->getAllArgValues(OPT_B)[0], "");
+  InputArgList AL = T.ParseArgs(MyArgs, MAI, MAC);
+  EXPECT_EQ(AL.size(), 1U);
+  EXPECT_TRUE(AL.hasArg(OPT_B));
+  EXPECT_EQ(AL.getAllArgValues(OPT_B).size(), 1U);
+  EXPECT_EQ(AL.getAllArgValues(OPT_B)[0], "");
 }