[llvm] r240345 - Modify ParseArgs to return the InputArgList by value - there's no need for dynamic allocation/ownership here
David Blaikie
dblaikie at gmail.com
Mon Jun 22 15:06:37 PDT 2015
Author: dblaikie
Date: Mon Jun 22 17:06:37 2015
New Revision: 240345
URL: http://llvm.org/viewvc/llvm-project?rev=240345&view=rev
Log:
Modify ParseArgs to return the InputArgList by value - there's no need for dynamic allocation/ownership here
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.
Modified:
llvm/trunk/include/llvm/Option/ArgList.h
llvm/trunk/include/llvm/Option/OptTable.h
llvm/trunk/lib/LibDriver/LibDriver.cpp
llvm/trunk/lib/Option/OptTable.cpp
llvm/trunk/unittests/Option/OptionParsingTest.cpp
Modified: llvm/trunk/include/llvm/Option/ArgList.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Option/ArgList.h?rev=240345&r1=240344&r2=240345&view=diff
==============================================================================
--- llvm/trunk/include/llvm/Option/ArgList.h (original)
+++ llvm/trunk/include/llvm/Option/ArgList.h Mon Jun 22 17:06:37 2015
@@ -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 {
Modified: llvm/trunk/include/llvm/Option/OptTable.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Option/OptTable.h?rev=240345&r1=240344&r2=240345&view=diff
==============================================================================
--- llvm/trunk/include/llvm/Option/OptTable.h (original)
+++ llvm/trunk/include/llvm/Option/OptTable.h Mon Jun 22 17:06:37 2015
@@ -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.
///
Modified: llvm/trunk/lib/LibDriver/LibDriver.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/LibDriver/LibDriver.cpp?rev=240345&r1=240344&r2=240345&view=diff
==============================================================================
--- llvm/trunk/lib/LibDriver/LibDriver.cpp (original)
+++ llvm/trunk/lib/LibDriver/LibDriver.cpp Mon Jun 22 17:06:37 2015
@@ -113,27 +113,27 @@ int llvm::libDriverMain(llvm::ArrayRef<c
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<c
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];
Modified: llvm/trunk/lib/Option/OptTable.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Option/OptTable.cpp?rev=240345&r1=240344&r2=240345&view=diff
==============================================================================
--- llvm/trunk/lib/Option/OptTable.cpp (original)
+++ llvm/trunk/lib/Option/OptTable.cpp Mon Jun 22 17:06:37 2015
@@ -247,12 +247,12 @@ Arg *OptTable::ParseOneArg(const ArgList
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(ArrayR
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(ArrayR
break;
}
- Args->append(A);
+ Args.append(A);
}
- return Args;
+ return std::move(Args);
}
static std::string getOptionHelpName(const OptTable &Opts, OptSpecifier Id) {
Modified: llvm/trunk/unittests/Option/OptionParsingTest.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Option/OptionParsingTest.cpp?rev=240345&r1=240344&r2=240345&view=diff
==============================================================================
--- llvm/trunk/unittests/Option/OptionParsingTest.cpp (original)
+++ llvm/trunk/unittests/Option/OptionParsingTest.cpp Mon Jun 22 17:06:37 2015
@@ -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], "");
}
More information about the llvm-commits
mailing list