On Mon, May 14, 2012 at 11:43 PM, David Blaikie <[email protected]> wrote: > Excuse my slightly delayed feedback, > > On Tue, Apr 17, 2012 at 9:54 AM, Manuel Klimek <[email protected]> wrote: >> Author: klimek >> Date: Tue Apr 17 11:54:26 2012 >> New Revision: 154929 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=154929&view=rev >> Log: >> Switches the JSONCompilationDatabase to use the YAML parser. >> This will allow us to delete the JSON parser from llvm. >> >> The biggest change is a general change of strategy - instead >> of storing StringRef's to the values for the command line and >> directory in the input buffer, we store ScalarNode*'s. The >> reason is that the YAML parser's getRawValue on ScalarNodes >> returns a string that includes the quotes in case of double >> quoted strings. >> >> For the same reason we're removing the JSON parsing part of >> the command line parsing - this means an extra copy for a >> command line when it is requested (and only when it is requested). >> >> >> Modified: >> cfe/trunk/include/clang/Tooling/CompilationDatabase.h >> cfe/trunk/lib/Tooling/CompilationDatabase.cpp >> >> Modified: cfe/trunk/include/clang/Tooling/CompilationDatabase.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Tooling/CompilationDatabase.h?rev=154929&r1=154928&r2=154929&view=diff >> ============================================================================== >> --- cfe/trunk/include/clang/Tooling/CompilationDatabase.h (original) >> +++ cfe/trunk/include/clang/Tooling/CompilationDatabase.h Tue Apr 17 >> 11:54:26 2012 >> @@ -34,13 +34,11 @@ >> #include "llvm/ADT/StringMap.h" >> #include "llvm/ADT/StringRef.h" >> #include "llvm/Support/MemoryBuffer.h" >> +#include "llvm/Support/SourceMgr.h" >> +#include "llvm/Support/YAMLParser.h" >> #include <string> >> #include <vector> >> >> -namespace llvm { >> -class MemoryBuffer; >> -} // end namespace llvm >> - >> namespace clang { >> namespace tooling { >> >> @@ -139,7 +137,7 @@ >> private: >> /// \brief Constructs a JSON compilation database on a memory buffer. >> JSONCompilationDatabase(llvm::MemoryBuffer *Database) >> - : Database(Database) {} >> + : Database(Database), YAMLStream(Database->getBuffer(), SM) {} >> >> /// \brief Parses the database file and creates the index. >> /// >> @@ -147,14 +145,17 @@ >> /// failed. >> bool parse(std::string &ErrorMessage); >> >> - // Tuple (directory, commandline) where 'commandline' is a JSON escaped >> bash >> - // escaped command line. >> - typedef std::pair<StringRef, StringRef> CompileCommandRef; >> + // Tuple (directory, commandline) where 'commandline' pointing to the >> + // corresponding nodes in the YAML stream. >> + typedef std::pair<llvm::yaml::ScalarNode*, >> + llvm::yaml::ScalarNode*> CompileCommandRef; >> >> // Maps file paths to the compile command lines for that file. >> llvm::StringMap< std::vector<CompileCommandRef> > IndexByFile; >> >> llvm::OwningPtr<llvm::MemoryBuffer> Database; >> + llvm::SourceMgr SM; >> + llvm::yaml::Stream YAMLStream; >> }; >> >> } // end namespace tooling >> >> Modified: cfe/trunk/lib/Tooling/CompilationDatabase.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/CompilationDatabase.cpp?rev=154929&r1=154928&r2=154929&view=diff >> ============================================================================== >> --- cfe/trunk/lib/Tooling/CompilationDatabase.cpp (original) >> +++ cfe/trunk/lib/Tooling/CompilationDatabase.cpp Tue Apr 17 11:54:26 2012 >> @@ -13,7 +13,7 @@ >> >> #include "clang/Tooling/CompilationDatabase.h" >> #include "llvm/ADT/SmallString.h" >> -#include "llvm/Support/JSONParser.h" >> +#include "llvm/Support/YAMLParser.h" >> #include "llvm/Support/Path.h" >> #include "llvm/Support/system_error.h" >> >> @@ -22,10 +22,10 @@ >> >> namespace { >> >> -/// \brief A parser for JSON escaped strings of command line arguments. >> +/// \brief A parser for escaped strings of command line arguments. >> /// >> /// Assumes \-escaping for quoted arguments (see the documentation of >> -/// unescapeJSONCommandLine(...)). >> +/// unescapeCommandLine(...)). >> class CommandLineArgumentParser { >> public: >> CommandLineArgumentParser(StringRef CommandLine) >> @@ -90,9 +90,6 @@ >> >> bool next() { >> ++Position; >> - if (Position == Input.end()) return false; >> - // Remove the JSON escaping first. This is done unconditionally. >> - if (*Position == '\\') ++Position; >> return Position != Input.end(); >> } >> >> @@ -101,9 +98,9 @@ >> std::vector<std::string> CommandLine; >> }; >> >> -std::vector<std::string> unescapeJSONCommandLine( >> - StringRef JSONEscapedCommandLine) { >> - CommandLineArgumentParser parser(JSONEscapedCommandLine); >> +std::vector<std::string> unescapeCommandLine( >> + StringRef EscapedCommandLine) { >> + CommandLineArgumentParser parser(EscapedCommandLine); >> return parser.parse(); >> } >> >> @@ -162,65 +159,77 @@ >> const std::vector<CompileCommandRef> &CommandsRef = >> CommandsRefI->getValue(); >> std::vector<CompileCommand> Commands; >> for (int I = 0, E = CommandsRef.size(); I != E; ++I) { >> + llvm::SmallString<8> DirectoryStorage; >> + llvm::SmallString<1024> CommandStorage; >> Commands.push_back(CompileCommand( >> // FIXME: Escape correctly: >> - CommandsRef[I].first, >> - unescapeJSONCommandLine(CommandsRef[I].second))); >> + CommandsRef[I].first->getValue(DirectoryStorage), >> + >> unescapeCommandLine(CommandsRef[I].second->getValue(CommandStorage)))); >> } >> return Commands; >> } >> >> bool JSONCompilationDatabase::parse(std::string &ErrorMessage) { >> - llvm::SourceMgr SM; >> - llvm::JSONParser Parser(Database->getBuffer(), &SM); >> - llvm::JSONValue *Root = Parser.parseRoot(); >> + llvm::yaml::document_iterator I = YAMLStream.begin(); >> + if (I == YAMLStream.end()) { >> + ErrorMessage = "Error while parsing YAML."; >> + return false; >> + } >> + llvm::yaml::Node *Root = I->getRoot(); >> if (Root == NULL) { >> - ErrorMessage = "Error while parsing JSON."; >> + ErrorMessage = "Error while parsing YAML."; >> return false; >> } >> - llvm::JSONArray *Array = dyn_cast<llvm::JSONArray>(Root); >> + llvm::yaml::SequenceNode *Array = >> + llvm::dyn_cast<llvm::yaml::SequenceNode>(Root); >> if (Array == NULL) { >> ErrorMessage = "Expected array."; >> return false; >> } >> - for (llvm::JSONArray::const_iterator AI = Array->begin(), AE = >> Array->end(); >> + for (llvm::yaml::SequenceNode::iterator AI = Array->begin(), >> + AE = Array->end(); >> AI != AE; ++AI) { >> - const llvm::JSONObject *Object = dyn_cast<llvm::JSONObject>(*AI); >> + llvm::yaml::MappingNode *Object = >> + llvm::dyn_cast<llvm::yaml::MappingNode>(&*AI); >> if (Object == NULL) { >> ErrorMessage = "Expected object."; >> return false; >> } >> - StringRef EntryDirectory; >> - StringRef EntryFile; >> - StringRef EntryCommand; >> - for (llvm::JSONObject::const_iterator KVI = Object->begin(), >> - KVE = Object->end(); >> + llvm::yaml::ScalarNode *Directory; >> + llvm::yaml::ScalarNode *Command; >> + llvm::SmallString<8> FileStorage; >> + llvm::StringRef File; >> + for (llvm::yaml::MappingNode::iterator KVI = Object->begin(), >> + KVE = Object->end(); >> KVI != KVE; ++KVI) { >> - const llvm::JSONValue *Value = (*KVI)->Value; >> + llvm::yaml::Node *Value = (*KVI).getValue(); >> if (Value == NULL) { >> ErrorMessage = "Expected value."; >> return false; >> } >> - const llvm::JSONString *ValueString = >> - dyn_cast<llvm::JSONString>(Value); >> + llvm::yaml::ScalarNode *ValueString = >> + llvm::dyn_cast<llvm::yaml::ScalarNode>(Value); > > Should this be a (non-dyn) llvm::cast? Or should it be null checked > before it's dereferenced 5 lines down? > >> if (ValueString == NULL) { >> ErrorMessage = "Expected string as value."; >> return false; >> } >> - if ((*KVI)->Key->getRawText() == "directory") { >> - EntryDirectory = ValueString->getRawText(); >> - } else if ((*KVI)->Key->getRawText() == "file") { >> - EntryFile = ValueString->getRawText(); >> - } else if ((*KVI)->Key->getRawText() == "command") { >> - EntryCommand = ValueString->getRawText(); >> + llvm::yaml::ScalarNode *KeyString = >> + llvm::dyn_cast<llvm::yaml::ScalarNode>((*KVI).getKey()); >> + llvm::SmallString<8> KeyStorage; >> + if (KeyString->getValue(KeyStorage) == "directory") { >> + Directory = ValueString; >> + } else if (KeyString->getValue(KeyStorage) == "command") { >> + Command = ValueString; >> + } else if (KeyString->getValue(KeyStorage) == "file") { >> + File = ValueString->getValue(FileStorage); >> } else { >> - ErrorMessage = (Twine("Unknown key: \"") + >> - (*KVI)->Key->getRawText() + "\"").str(); >> + ErrorMessage = ("Unknown key: \"" + >> + KeyString->getRawValue() + "\"").str(); >> return false; >> } >> } >> - IndexByFile[EntryFile].push_back( >> - CompileCommandRef(EntryDirectory, EntryCommand)); >> + IndexByFile[File].push_back( >> + CompileCommandRef(Directory, Command)); > > Are 'Directory' and 'Command' guaranteed to have been initialized by > the above loop? (perhaps the file has been verified to contain a > 'directory' and 'command' key by some previous code? I haven't looked > closely) > > GCC's maybe-uninitialized mentioned this so I just thought I'd have a > look around at it.
Thanks! All fixed in r156814. Found some missing tests after a migration to a much broader interface ;) > > - David > >> } >> return true; >> } >> >> >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
