[cfe-commits] r154929 - in /cfe/trunk: include/clang/Tooling/CompilationDatabase.h lib/Tooling/CompilationDatabase.cpp
Manuel Klimek
klimek at google.com
Tue May 15 04:48:14 PDT 2012
On Mon, May 14, 2012 at 11:43 PM, David Blaikie <dblaikie at gmail.com> wrote:
> Excuse my slightly delayed feedback,
>
> On Tue, Apr 17, 2012 at 9:54 AM, Manuel Klimek <klimek at google.com> 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
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list