[PATCH] D27138: Extend CompilationDatabase by a field for the output filename
This revision was automatically updated to reflect the committed changes.
Closed by commit rL288436: Extend CompilationDatabase by a field for the output
filename (authored by joerg).
Changed prior to commit:
https://reviews.llvm.org/D27138?vs=79320=79993#toc
Repository:
rL LLVM
https://reviews.llvm.org/D27138
Files:
cfe/trunk/docs/JSONCompilationDatabase.rst
cfe/trunk/include/clang/Tooling/CompilationDatabase.h
cfe/trunk/include/clang/Tooling/JSONCompilationDatabase.h
cfe/trunk/lib/Tooling/CompilationDatabase.cpp
cfe/trunk/lib/Tooling/JSONCompilationDatabase.cpp
cfe/trunk/unittests/Tooling/CompilationDatabaseTest.cpp
Index: cfe/trunk/lib/Tooling/CompilationDatabase.cpp
===
--- cfe/trunk/lib/Tooling/CompilationDatabase.cpp
+++ cfe/trunk/lib/Tooling/CompilationDatabase.cpp
@@ -300,7 +300,8 @@
ToolCommandLine.insert(ToolCommandLine.end(),
CommandLine.begin(), CommandLine.end());
CompileCommands.emplace_back(Directory, StringRef(),
- std::move(ToolCommandLine));
+ std::move(ToolCommandLine),
+ StringRef());
}
std::vector
Index: cfe/trunk/lib/Tooling/JSONCompilationDatabase.cpp
===
--- cfe/trunk/lib/Tooling/JSONCompilationDatabase.cpp
+++ cfe/trunk/lib/Tooling/JSONCompilationDatabase.cpp
@@ -257,10 +257,13 @@
for (int I = 0, E = CommandsRef.size(); I != E; ++I) {
SmallString<8> DirectoryStorage;
SmallString<32> FilenameStorage;
+SmallString<32> OutputStorage;
+auto Output = std::get<3>(CommandsRef[I]);
Commands.emplace_back(
std::get<0>(CommandsRef[I])->getValue(DirectoryStorage),
std::get<1>(CommandsRef[I])->getValue(FilenameStorage),
-nodeToCommandLine(Syntax, std::get<2>(CommandsRef[I])));
+nodeToCommandLine(Syntax, std::get<2>(CommandsRef[I])),
+Output ? Output->getValue(OutputStorage) : "");
}
}
@@ -289,6 +292,7 @@
llvm::yaml::ScalarNode *Directory = nullptr;
llvm::Optional Command;
llvm::yaml::ScalarNode *File = nullptr;
+llvm::yaml::ScalarNode *Output = nullptr;
for (auto& NextKeyValue : *Object) {
llvm::yaml::ScalarNode *KeyString =
dyn_cast(NextKeyValue.getKey());
@@ -331,6 +335,8 @@
Command = std::vector(1, ValueString);
} else if (KeyValue == "file") {
File = ValueString;
+ } else if (KeyValue == "output") {
+Output = ValueString;
} else {
ErrorMessage = ("Unknown key: \"" +
KeyString->getRawValue() + "\"").str();
@@ -361,7 +367,7 @@
} else {
llvm::sys::path::native(FileName, NativeFilePath);
}
-auto Cmd = CompileCommandRef(Directory, File, *Command);
+auto Cmd = CompileCommandRef(Directory, File, *Command, Output);
IndexByFile[NativeFilePath].push_back(Cmd);
AllCommands.push_back(Cmd);
MatchTrie.insert(NativeFilePath);
Index: cfe/trunk/unittests/Tooling/CompilationDatabaseTest.cpp
===
--- cfe/trunk/unittests/Tooling/CompilationDatabaseTest.cpp
+++ cfe/trunk/unittests/Tooling/CompilationDatabaseTest.cpp
@@ -44,6 +44,7 @@
expectFailure("[{\"directory\":\"\",\"command\":[],\"file\":\"\"}]", "Command not string");
expectFailure("[{\"directory\":\"\",\"arguments\":[[]],\"file\":\"\"}]",
"Arguments contain non-string");
+ expectFailure("[{\"output\":[]}]", "Expected strings as value.");
}
static std::vector getAllFiles(StringRef JSONDatabase,
@@ -105,16 +106,19 @@
StringRef Directory1("//net/dir1");
StringRef FileName1("file1");
StringRef Command1("command1");
+ StringRef Output1("file1.o");
StringRef Directory2("//net/dir2");
StringRef FileName2("file2");
StringRef Command2("command2");
+ StringRef Output2("");
std::vector Commands = getAllCompileCommands(
JSONCommandLineSyntax::Gnu,
("[{\"directory\":\"" + Directory1 + "\"," + "\"command\":\"" + Command1 +
"\","
"\"file\":\"" +
- FileName1 + "\"},"
+ FileName1 + "\", \"output\":\"" +
+ Output1 + "\"},"
" {\"directory\":\"" +
Directory2 + "\"," + "\"command\":\"" + Command2 + "\","
"\"file\":\"" +
@@ -124,10 +128,12 @@
EXPECT_EQ(2U, Commands.size()) << ErrorMessage;
EXPECT_EQ(Directory1, Commands[0].Directory) << ErrorMessage;
EXPECT_EQ(FileName1, Commands[0].Filename) << ErrorMessage;
+ EXPECT_EQ(Output1, Commands[0].Output) << ErrorMessage;
ASSERT_EQ(1u, Commands[0].CommandLine.size());
EXPECT_EQ(Command1, Commands[0].CommandLine[0]) << ErrorMessage;
EXPECT_EQ(Directory2, Commands[1].Directory) << ErrorMessage;
EXPECT_EQ(FileName2, Commands[1].Filename) <<
[PATCH] D27138: Extend CompilationDatabase by a field for the output filename
klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D27138#607859, @joerg wrote: > Which struct are we talking about, `CompileCommandRef` or `CompileCommand`? > It is a pointer in the former and a plain StringRef in the latter. I don't > think making it a pointer in both is an advantage, i.e. distinguishing empty > input from missing field is not valuable in my opinion. Ok, you convinced me. LG then. Repository: rL LLVM https://reviews.llvm.org/D27138 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27138: Extend CompilationDatabase by a field for the output filename
joerg added a comment. Which struct are we talking about, `CompileCommandRef` or `CompileCommand`? It is a pointer in the former and a plain StringRef in the latter. I don't think making it a pointer in both is an advantage, i.e. distinguishing empty input from missing field is not valuable in my opinion. Repository: rL LLVM https://reviews.llvm.org/D27138 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27138: Extend CompilationDatabase by a field for the output filename
klimek added a comment.
In https://reviews.llvm.org/D27138#607786, @joerg wrote:
> It's not the directory, but the output file. That's optional since it is a
> new addition and I don't want to invalidate all existing JSON databases.
It seems like we're talking past each other. I'm not suggesting to invalidate
all existing JSON databases. I'm suggesting:
1. if it doesn't exist, have nullptr in the struct
2. if it exists and is empty (""), have the empty string in the struct (and
potentially give the user an error)
3. if it exists and is non-empty, all is good
Repository:
rL LLVM
https://reviews.llvm.org/D27138
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27138: Extend CompilationDatabase by a field for the output filename
joerg added a comment. It's not the directory, but the output file. That's optional since it is a new addition and I don't want to invalidate all existing JSON databases. Repository: rL LLVM https://reviews.llvm.org/D27138 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27138: Extend CompilationDatabase by a field for the output filename
klimek added inline comments. Comment at: lib/Tooling/JSONCompilationDatabase.cpp:266 +nodeToCommandLine(Syntax, std::get<2>(CommandsRef[I])), +Output ? Output->getValue(OutputStorage) : ""); } joerg wrote: > klimek wrote: > > joerg wrote: > > > klimek wrote: > > > > joerg wrote: > > > > > klimek wrote: > > > > > > Optional: I'd probably let the nodeToCommandLine handle the null > > > > > > value and make this code more straight forward? > > > > > I couldn't find a way to create a synthetic node without changing the > > > > > YAML API. > > > > I'm probably missing something - why would we need a synthetic node? > > > > Can't we just put nullptr into the vector? > > > That's what I am doing and why this line checks output :) > > Ok, let's ask differently: why is it a problem if we put a nullptr into the > > array? > I think it just adds unnecessary complexity. An empty file name is not a > valid output, so "" vs Optional has the same result. I'd have prefered to > keep this complexity inside the JSON parser, but that would have meant > creating a synthetic node with value "" and there is no API for that at the > moment. Don't we thus want to actually be able to err out on a higher level when getting an empty output dir, but be OK with an unspecified (-> nullptr) one? Repository: rL LLVM https://reviews.llvm.org/D27138 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27138: Extend CompilationDatabase by a field for the output filename
joerg added a comment. Can I interprete that as LGTM otherwise? Repository: rL LLVM https://reviews.llvm.org/D27138 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27138: Extend CompilationDatabase by a field for the output filename
joerg added inline comments. Comment at: lib/Tooling/JSONCompilationDatabase.cpp:266 +nodeToCommandLine(Syntax, std::get<2>(CommandsRef[I])), +Output ? Output->getValue(OutputStorage) : ""); } klimek wrote: > joerg wrote: > > klimek wrote: > > > joerg wrote: > > > > klimek wrote: > > > > > Optional: I'd probably let the nodeToCommandLine handle the null > > > > > value and make this code more straight forward? > > > > I couldn't find a way to create a synthetic node without changing the > > > > YAML API. > > > I'm probably missing something - why would we need a synthetic node? > > > Can't we just put nullptr into the vector? > > That's what I am doing and why this line checks output :) > Ok, let's ask differently: why is it a problem if we put a nullptr into the > array? I think it just adds unnecessary complexity. An empty file name is not a valid output, so "" vs Optional has the same result. I'd have prefered to keep this complexity inside the JSON parser, but that would have meant creating a synthetic node with value "" and there is no API for that at the moment. Repository: rL LLVM https://reviews.llvm.org/D27138 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27138: Extend CompilationDatabase by a field for the output filename
klimek added inline comments. Comment at: lib/Tooling/JSONCompilationDatabase.cpp:266 +nodeToCommandLine(Syntax, std::get<2>(CommandsRef[I])), +Output ? Output->getValue(OutputStorage) : ""); } joerg wrote: > klimek wrote: > > joerg wrote: > > > klimek wrote: > > > > Optional: I'd probably let the nodeToCommandLine handle the null value > > > > and make this code more straight forward? > > > I couldn't find a way to create a synthetic node without changing the > > > YAML API. > > I'm probably missing something - why would we need a synthetic node? Can't > > we just put nullptr into the vector? > That's what I am doing and why this line checks output :) Ok, let's ask differently: why is it a problem if we put a nullptr into the array? Repository: rL LLVM https://reviews.llvm.org/D27138 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27138: Extend CompilationDatabase by a field for the output filename
joerg added inline comments. Comment at: lib/Tooling/JSONCompilationDatabase.cpp:266 +nodeToCommandLine(Syntax, std::get<2>(CommandsRef[I])), +Output ? Output->getValue(OutputStorage) : ""); } klimek wrote: > joerg wrote: > > klimek wrote: > > > Optional: I'd probably let the nodeToCommandLine handle the null value > > > and make this code more straight forward? > > I couldn't find a way to create a synthetic node without changing the YAML > > API. > I'm probably missing something - why would we need a synthetic node? Can't we > just put nullptr into the vector? That's what I am doing and why this line checks output :) Repository: rL LLVM https://reviews.llvm.org/D27138 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27138: Extend CompilationDatabase by a field for the output filename
klimek added inline comments. Comment at: lib/Tooling/JSONCompilationDatabase.cpp:266 +nodeToCommandLine(Syntax, std::get<2>(CommandsRef[I])), +Output ? Output->getValue(OutputStorage) : ""); } joerg wrote: > klimek wrote: > > Optional: I'd probably let the nodeToCommandLine handle the null value and > > make this code more straight forward? > I couldn't find a way to create a synthetic node without changing the YAML > API. I'm probably missing something - why would we need a synthetic node? Can't we just put nullptr into the vector? Repository: rL LLVM https://reviews.llvm.org/D27138 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27138: Extend CompilationDatabase by a field for the output filename
joerg added inline comments. Comment at: lib/Tooling/JSONCompilationDatabase.cpp:266 +nodeToCommandLine(Syntax, std::get<2>(CommandsRef[I])), +Output ? Output->getValue(OutputStorage) : ""); } klimek wrote: > Optional: I'd probably let the nodeToCommandLine handle the null value and > make this code more straight forward? I couldn't find a way to create a synthetic node without changing the YAML API. Repository: rL LLVM https://reviews.llvm.org/D27138 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27138: Extend CompilationDatabase by a field for the output filename
klimek added inline comments. Comment at: lib/Tooling/JSONCompilationDatabase.cpp:266 +nodeToCommandLine(Syntax, std::get<2>(CommandsRef[I])), +Output ? Output->getValue(OutputStorage) : ""); } Optional: I'd probably let the nodeToCommandLine handle the null value and make this code more straight forward? Repository: rL LLVM https://reviews.llvm.org/D27138 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27138: Extend CompilationDatabase by a field for the output filename
joerg created this revision.
joerg added a reviewer: klimek.
joerg added a subscriber: cfe-commits.
joerg set the repository for this revision to rL LLVM.
In bigger projects like an Operating System, the same source code is often
compiled in slightly different ways. This could be the difference between PIC
and non-PIC code for static vs dynamic libraries, it could also be the
difference between size optimised versions of tools for ramdisk images. At the
moment, the compilation database has no way to distinguish such cases. As first
step, add a field in the JSON format for it and process it accordingly.
Repository:
rL LLVM
https://reviews.llvm.org/D27138
Files:
docs/JSONCompilationDatabase.rst
include/clang/Tooling/CompilationDatabase.h
include/clang/Tooling/JSONCompilationDatabase.h
lib/Tooling/CompilationDatabase.cpp
lib/Tooling/JSONCompilationDatabase.cpp
unittests/Tooling/CompilationDatabaseTest.cpp
Index: docs/JSONCompilationDatabase.rst
===
--- docs/JSONCompilationDatabase.rst
+++ docs/JSONCompilationDatabase.rst
@@ -80,6 +80,9 @@
supported.
- **arguments:** The compile command executed as list of strings.
Either **arguments** or **command** is required.
+- **output:** The name of the output created by this compilation step.
+ This field is optional. It can be used to distinguish different processing
+ modes of the same input file.
Build System Integration
Index: include/clang/Tooling/CompilationDatabase.h
===
--- include/clang/Tooling/CompilationDatabase.h
+++ include/clang/Tooling/CompilationDatabase.h
@@ -43,10 +43,11 @@
struct CompileCommand {
CompileCommand() {}
CompileCommand(Twine Directory, Twine Filename,
- std::vector CommandLine)
+ std::vector CommandLine, Twine Output)
: Directory(Directory.str()),
Filename(Filename.str()),
-CommandLine(std::move(CommandLine)) {}
+CommandLine(std::move(CommandLine)),
+Output(Output.str()){}
/// \brief The working directory the command was executed from.
std::string Directory;
@@ -57,6 +58,9 @@
/// \brief The command line that was executed.
std::vector CommandLine;
+ /// The output file associated with the command.
+ std::string Output;
+
/// \brief An optional mapping from each file's path to its content for all
/// files needed for the compilation that are not available via the file
/// system.
Index: include/clang/Tooling/JSONCompilationDatabase.h
===
--- include/clang/Tooling/JSONCompilationDatabase.h
+++ include/clang/Tooling/JSONCompilationDatabase.h
@@ -103,15 +103,17 @@
/// failed.
bool parse(std::string );
- // Tuple (directory, filename, commandline) where 'commandline' points to the
- // corresponding scalar nodes in the YAML stream.
+ // Tuple (directory, filename, commandline, output) where 'commandline'
+ // points to the corresponding scalar nodes in the YAML stream.
// If the command line contains a single argument, it is a shell-escaped
// command line.
// Otherwise, each entry in the command line vector is a literal
// argument to the compiler.
+ // The output field may be a nullptr.
typedef std::tuple> CompileCommandRef;
+ std::vector,
+ llvm::yaml::ScalarNode *> CompileCommandRef;
/// \brief Converts the given array of CompileCommandRefs to CompileCommands.
void getCommands(ArrayRef CommandsRef,
Index: lib/Tooling/CompilationDatabase.cpp
===
--- lib/Tooling/CompilationDatabase.cpp
+++ lib/Tooling/CompilationDatabase.cpp
@@ -300,7 +300,8 @@
ToolCommandLine.insert(ToolCommandLine.end(),
CommandLine.begin(), CommandLine.end());
CompileCommands.emplace_back(Directory, StringRef(),
- std::move(ToolCommandLine));
+ std::move(ToolCommandLine),
+ StringRef());
}
std::vector
Index: lib/Tooling/JSONCompilationDatabase.cpp
===
--- lib/Tooling/JSONCompilationDatabase.cpp
+++ lib/Tooling/JSONCompilationDatabase.cpp
@@ -257,10 +257,13 @@
for (int I = 0, E = CommandsRef.size(); I != E; ++I) {
SmallString<8> DirectoryStorage;
SmallString<32> FilenameStorage;
+SmallString<32> OutputStorage;
+auto Output = std::get<3>(CommandsRef[I]);
Commands.emplace_back(
std::get<0>(CommandsRef[I])->getValue(DirectoryStorage),
std::get<1>(CommandsRef[I])->getValue(FilenameStorage),
-nodeToCommandLine(Syntax, std::get<2>(CommandsRef[I])));
+nodeToCommandLine(Syntax, std::get<2>(CommandsRef[I])),
+Output ?
