From 3f8a26f6fe16cc76c98ab21db2c600bd7defbbaa Mon Sep 17 00:00:00 2001 From: Sean Silva Date: Fri, 15 Aug 2014 23:18:33 +0000 Subject: [PATCH] [Support] Promote cl::StringSaver to a separate utility This class is generally useful. In breaking it out, the primary change is that it has been made non-virtual. It seems like being abstract led to there being 3 different (2 in llvm + 1 in clang) concrete implementations which disagreed about the ownership of the saved strings (see the manual call to free() in the unittest StrDupSaver; yes this is different from the CommandLine.cpp StrDupSaver which owns the stored strings; which is different from Clang's StringSetSaver which just holds a reference to a std::set which owns the strings). I've identified 2 other places in the codebase that are open-coding this pattern: memcpy(Alloc.Allocate(strlen(S)+1), S, strlen(S)+1) I'll be switching them over. They are * llvm::sys::Process::GetArgumentVector * The StringAllocator member of YAMLIO's Input class This also will allow simplifying Clang's driver.cpp quite a bit. Let me know if there are any other places that could benefit from StringSaver. I'm also thinking of adding a saveStringRef member for getting a stable StringRef. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@215784 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/llvm/Support/CommandLine.h | 10 +------- include/llvm/Support/StringSaver.h | 33 ++++++++++++++++++++++++++ lib/Support/CommandLine.cpp | 34 ++++++--------------------- unittests/Support/CommandLineTest.cpp | 11 ++------- 4 files changed, 43 insertions(+), 45 deletions(-) create mode 100644 include/llvm/Support/StringSaver.h diff --git a/include/llvm/Support/CommandLine.h b/include/llvm/Support/CommandLine.h index fdd901200fe..ac25c59b904 100644 --- a/include/llvm/Support/CommandLine.h +++ b/include/llvm/Support/CommandLine.h @@ -24,6 +24,7 @@ #include "llvm/ADT/StringMap.h" #include "llvm/ADT/Twine.h" #include "llvm/Support/Compiler.h" +#include "llvm/Support/StringSaver.h" #include #include #include @@ -1772,15 +1773,6 @@ void getRegisteredOptions(StringMap &Map); // Standalone command line processing utilities. // -/// \brief Saves strings in the inheritor's stable storage and returns a stable -/// raw character pointer. -class StringSaver { - virtual void anchor(); -public: - virtual const char *SaveString(const char *Str) = 0; - virtual ~StringSaver() {}; // Pacify -Wnon-virtual-dtor. -}; - /// \brief Tokenizes a command line that can contain escapes and quotes. // /// The quoting rules match those used by GCC and other tools that use diff --git a/include/llvm/Support/StringSaver.h b/include/llvm/Support/StringSaver.h new file mode 100644 index 00000000000..8d3dc129edc --- /dev/null +++ b/include/llvm/Support/StringSaver.h @@ -0,0 +1,33 @@ +//===- llvm/Support/StringSaver.h - Stable storage for strings --*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_SUPPORT_STRINGSAVER_H +#define LLVM_SUPPORT_STRINGSAVER_H + +#include "llvm/Support/Allocator.h" +#include + +namespace llvm { + +/// \brief Saves strings in stable storage that it owns. +class StringSaver { + BumpPtrAllocator Alloc; + +public: + const char *saveCStr(const char *CStr) { + auto Len = std::strlen(CStr) + 1; // Don't forget the NUL! + char *Buf = Alloc.Allocate(Len); + std::memcpy(Buf, CStr, Len); + return Buf; + } +}; + +} // end namespace llvm + +#endif diff --git a/lib/Support/CommandLine.cpp b/lib/Support/CommandLine.cpp index 4c1df5c47dd..dddcbf3f3d7 100644 --- a/lib/Support/CommandLine.cpp +++ b/lib/Support/CommandLine.cpp @@ -76,7 +76,6 @@ void parser::anchor() {} void parser::anchor() {} void parser::anchor() {} void parser::anchor() {} -void StringSaver::anchor() {} //===----------------------------------------------------------------------===// @@ -509,7 +508,7 @@ void cl::TokenizeGNUCommandLine(StringRef Src, StringSaver &Saver, // End the token if this is whitespace. if (isWhitespace(Src[I])) { if (!Token.empty()) - NewArgv.push_back(Saver.SaveString(Token.c_str())); + NewArgv.push_back(Saver.saveCStr(Token.c_str())); Token.clear(); continue; } @@ -520,7 +519,7 @@ void cl::TokenizeGNUCommandLine(StringRef Src, StringSaver &Saver, // Append the last token after hitting EOF with no whitespace. if (!Token.empty()) - NewArgv.push_back(Saver.SaveString(Token.c_str())); + NewArgv.push_back(Saver.saveCStr(Token.c_str())); } /// Backslashes are interpreted in a rather complicated way in the Windows-style @@ -593,7 +592,7 @@ void cl::TokenizeWindowsCommandLine(StringRef Src, StringSaver &Saver, if (State == UNQUOTED) { // Whitespace means the end of the token. if (isWhitespace(Src[I])) { - NewArgv.push_back(Saver.SaveString(Token.c_str())); + NewArgv.push_back(Saver.saveCStr(Token.c_str())); Token.clear(); State = INIT; continue; @@ -625,7 +624,7 @@ void cl::TokenizeWindowsCommandLine(StringRef Src, StringSaver &Saver, } // Append the last token after hitting EOF with no whitespace. if (!Token.empty()) - NewArgv.push_back(Saver.SaveString(Token.c_str())); + NewArgv.push_back(Saver.saveCStr(Token.c_str())); } static bool ExpandResponseFile(const char *FName, StringSaver &Saver, @@ -691,25 +690,6 @@ bool cl::ExpandResponseFiles(StringSaver &Saver, TokenizerCallback Tokenizer, return AllExpanded; } -namespace { - class StrDupSaver : public StringSaver { - std::vector Dups; - public: - ~StrDupSaver() { - for (std::vector::iterator I = Dups.begin(), E = Dups.end(); - I != E; ++I) { - char *Dup = *I; - free(Dup); - } - } - const char *SaveString(const char *Str) override { - char *Dup = strdup(Str); - Dups.push_back(Dup); - return Dup; - } - }; -} - /// ParseEnvironmentOptions - An alternative entry point to the /// CommandLine library, which allows you to read the program's name /// from the caller (as PROGNAME) and its command-line arguments from @@ -729,8 +709,8 @@ void cl::ParseEnvironmentOptions(const char *progName, const char *envVar, // Get program's "name", which we wouldn't know without the caller // telling us. SmallVector newArgv; - StrDupSaver Saver; - newArgv.push_back(Saver.SaveString(progName)); + StringSaver Saver; + newArgv.push_back(Saver.saveCStr(progName)); // Parse the value of the environment variable into a "command line" // and hand it off to ParseCommandLineOptions(). @@ -754,7 +734,7 @@ void cl::ParseCommandLineOptions(int argc, const char * const *argv, SmallVector newArgv; for (int i = 0; i != argc; ++i) newArgv.push_back(argv[i]); - StrDupSaver Saver; + StringSaver Saver; ExpandResponseFiles(Saver, TokenizeGNUCommandLine, newArgv); argv = &newArgv[0]; argc = static_cast(newArgv.size()); diff --git a/unittests/Support/CommandLineTest.cpp b/unittests/Support/CommandLineTest.cpp index e4a1b67c47e..ffabaca4610 100644 --- a/unittests/Support/CommandLineTest.cpp +++ b/unittests/Support/CommandLineTest.cpp @@ -146,26 +146,19 @@ TEST(CommandLineTest, UseOptionCategory) { "Category."; } -class StrDupSaver : public cl::StringSaver { - const char *SaveString(const char *Str) override { - return strdup(Str); - } -}; - -typedef void ParserFunction(StringRef Source, llvm::cl::StringSaver &Saver, +typedef void ParserFunction(StringRef Source, StringSaver &Saver, SmallVectorImpl &NewArgv); void testCommandLineTokenizer(ParserFunction *parse, const char *Input, const char *const Output[], size_t OutputSize) { SmallVector Actual; - StrDupSaver Saver; + StringSaver Saver; parse(Input, Saver, Actual); EXPECT_EQ(OutputSize, Actual.size()); for (unsigned I = 0, E = Actual.size(); I != E; ++I) { if (I < OutputSize) EXPECT_STREQ(Output[I], Actual[I]); - free(const_cast(Actual[I])); } }