<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Aug 28, 2014 at 1:03 AM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div class="">On Wed, Aug 27, 2014 at 2:36 PM, Alexander Kornienko <span dir="ltr"><<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Author: alexfh<br>
Date: Wed Aug 27 16:36:39 2014<br>
New Revision: 216620<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=216620&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=216620&view=rev</a><br>
Log:<br>
Query CompilationDatabase right before running each compilation.<br>
<br>
Summary:<br>
Query CompilationDatabase right before running each compilation. This allows<br>
supporting compilation databases that change external state required for<br>
successful compilation.<br></blockquote><div><br></div></div><div>Could you give an example of this? It's not clear to me what this means.</div></div></div></div></blockquote><div><br></div><div>This means, that in some implementations the call to Compilations.getCompileCommands(File) may make changes in the file system to allow compilation of the File (e.g. generate headers from .td files). State of the file system required for compiling one file may be incompatible with the state required for compiling another file, so we actually need to run the tool on the file right after we call getCompileCommands for this file.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra">
<div class="gmail_quote"><div><div class="h5"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Reviewers: klimek, djasper<br>
<br>
Reviewed By: djasper<br>
<br>
Subscribers: klimek, cfe-commits<br>
<br>
Differential Revision: <a href="http://reviews.llvm.org/D5086" target="_blank">http://reviews.llvm.org/D5086</a><br>
<br>
Modified:<br>
cfe/trunk/include/clang/Tooling/Tooling.h<br>
cfe/trunk/lib/Tooling/Tooling.cpp<br>
<br>
Modified: cfe/trunk/include/clang/Tooling/Tooling.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Tooling/Tooling.h?rev=216620&r1=216619&r2=216620&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Tooling/Tooling.h?rev=216620&r1=216619&r2=216620&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Tooling/Tooling.h (original)<br>
+++ cfe/trunk/include/clang/Tooling/Tooling.h Wed Aug 27 16:36:39 2014<br>
@@ -293,8 +293,8 @@ class ClangTool {<br>
FileManager &getFiles() { return *Files; }<br>
<br>
private:<br>
- // We store compile commands as pair (file name, compile command).<br>
- std::vector< std::pair<std::string, CompileCommand> > CompileCommands;<br>
+ const CompilationDatabase &Compilations;<br>
+ std::vector<std::string> SourcePaths;<br>
<br>
llvm::IntrusiveRefCntPtr<FileManager> Files;<br>
// Contains a list of pairs (<file name>, <file content>).<br>
<br>
Modified: cfe/trunk/lib/Tooling/Tooling.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/Tooling.cpp?rev=216620&r1=216619&r2=216620&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/Tooling.cpp?rev=216620&r1=216619&r2=216620&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Tooling/Tooling.cpp (original)<br>
+++ cfe/trunk/lib/Tooling/Tooling.cpp Wed Aug 27 16:36:39 2014<br>
@@ -273,28 +273,10 @@ bool FrontendActionFactory::runInvocatio<br>
<br>
ClangTool::ClangTool(const CompilationDatabase &Compilations,<br>
ArrayRef<std::string> SourcePaths)<br>
- : Files(new FileManager(FileSystemOptions())), DiagConsumer(nullptr) {<br>
+ : Compilations(Compilations), SourcePaths(SourcePaths),<br>
+ Files(new FileManager(FileSystemOptions())), DiagConsumer(nullptr) {<br>
ArgsAdjusters.push_back(new ClangStripOutputAdjuster());<br>
ArgsAdjusters.push_back(new ClangSyntaxOnlyAdjuster());<br>
- for (const auto &SourcePath : SourcePaths) {<br>
- std::string File(getAbsolutePath(SourcePath));<br>
-<br>
- std::vector<CompileCommand> CompileCommandsForFile =<br>
- Compilations.getCompileCommands(File);<br>
- if (!CompileCommandsForFile.empty()) {<br>
- for (CompileCommand &CompileCommand : CompileCommandsForFile) {<br>
- CompileCommands.push_back(<br>
- std::make_pair(File, std::move(CompileCommand)));<br>
- }<br>
- } else {<br>
- // FIXME: There are two use cases here: doing a fuzzy<br>
- // "find . -name '*.cc' |xargs tool" match, where as a user I don't care<br>
- // about the .cc files that were not found, and the use case where I<br>
- // specify all files I want to run over explicitly, where this should<br>
- // be an error. We'll want to add an option for this.<br>
- llvm::errs() << "Skipping " << File << ". Compile command not found.\n";<br>
- }<br>
- }<br>
}<br>
<br>
void ClangTool::setDiagnosticConsumer(DiagnosticConsumer *D) {<br>
@@ -333,36 +315,49 @@ int ClangTool::run(ToolAction *Action) {<br>
llvm::sys::fs::getMainExecutable("clang_tool", &StaticSymbol);<br>
<br>
bool ProcessingFailed = false;<br>
- for (const auto &Command : CompileCommands) {<br>
- // FIXME: chdir is thread hostile; on the other hand, creating the same<br>
- // behavior as chdir is complex: chdir resolves the path once, thus<br>
- // guaranteeing that all subsequent relative path operations work<br>
- // on the same path the original chdir resulted in. This makes a difference<br>
- // for example on network filesystems, where symlinks might be switched<br>
- // during runtime of the tool. Fixing this depends on having a file system<br>
- // abstraction that allows openat() style interactions.<br>
- if (chdir(Command.second.Directory.c_str()))<br>
- llvm::report_fatal_error("Cannot chdir into \"" +<br>
- Twine(Command.second.Directory) + "\n!");<br>
- std::vector<std::string> CommandLine = Command.second.CommandLine;<br>
- for (ArgumentsAdjuster *Adjuster : ArgsAdjusters)<br>
- CommandLine = Adjuster->Adjust(CommandLine);<br>
- assert(!CommandLine.empty());<br>
- CommandLine[0] = MainExecutable;<br>
- // FIXME: We need a callback mechanism for the tool writer to output a<br>
- // customized message for each file.<br>
- DEBUG({<br>
- llvm::dbgs() << "Processing: " << Command.first << ".\n";<br>
- });<br>
- ToolInvocation Invocation(std::move(CommandLine), Action, Files.get());<br>
- Invocation.setDiagnosticConsumer(DiagConsumer);<br>
- for (const auto &MappedFile : MappedFileContents) {<br>
- Invocation.mapVirtualFile(MappedFile.first, MappedFile.second);<br>
+ for (const auto &SourcePath : SourcePaths) {<br>
+ std::string File(getAbsolutePath(SourcePath));<br>
+<br>
+ std::vector<CompileCommand> CompileCommandsForFile =<br>
+ Compilations.getCompileCommands(File);<br></blockquote><div><br></div></div></div><div>The analysis that I did in <a href="http://lists.cs.uiuc.edu/pipermail/cfe-dev/2013-September/032234.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/cfe-dev/2013-September/032234.html</a> still holds AFAIK, so this line here takes O(project size) time to execute for a JSON compilation database.</div>
</div></div></div></blockquote><div><br></div><div>Does it? JSONCompilationDatabase stores commands in llvm::StringMap indexed by filename. I would be surprised, if it took O(N) to fetch an item.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> The containing loop has O(project size) iterations when running a refactoring tool over a codebase, which means that the performance of this routine is now O(N^2), no? That seems unacceptable for our "default" suggested way for people to use clang tooling (JSON compilation database + single tool).</div>
</div></div></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra">
<div class="gmail_quote"><span class=""><font color="#888888">
<div><br></div><div><div>-- Sean Silva</div></div></font></span><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ if (CompileCommandsForFile.empty()) {<br>
+ // FIXME: There are two use cases here: doing a fuzzy<br>
+ // "find . -name '*.cc' |xargs tool" match, where as a user I don't care<br>
+ // about the .cc files that were not found, and the use case where I<br>
+ // specify all files I want to run over explicitly, where this should<br>
+ // be an error. We'll want to add an option for this.<br>
+ llvm::errs() << "Skipping " << File << ". Compile command not found.\n";<br>
+ continue;<br>
}<br>
- if (!Invocation.run()) {<br>
- // FIXME: Diagnostics should be used instead.<br>
- llvm::errs() << "Error while processing " << Command.first << ".\n";<br>
- ProcessingFailed = true;<br>
+ for (CompileCommand &CompileCommand : CompileCommandsForFile) {<br>
+ // FIXME: chdir is thread hostile; on the other hand, creating the same<br>
+ // behavior as chdir is complex: chdir resolves the path once, thus<br>
+ // guaranteeing that all subsequent relative path operations work<br>
+ // on the same path the original chdir resulted in. This makes a<br>
+ // difference for example on network filesystems, where symlinks might be<br>
+ // switched during runtime of the tool. Fixing this depends on having a<br>
+ // file system abstraction that allows openat() style interactions.<br>
+ if (chdir(CompileCommand.Directory.c_str()))<br>
+ llvm::report_fatal_error("Cannot chdir into \"" +<br>
+ Twine(CompileCommand.Directory) + "\n!");<br>
+ std::vector<std::string> CommandLine = CompileCommand.CommandLine;<br>
+ for (ArgumentsAdjuster *Adjuster : ArgsAdjusters)<br>
+ CommandLine = Adjuster->Adjust(CommandLine);<br>
+ assert(!CommandLine.empty());<br>
+ CommandLine[0] = MainExecutable;<br>
+ // FIXME: We need a callback mechanism for the tool writer to output a<br>
+ // customized message for each file.<br>
+ DEBUG({ llvm::dbgs() << "Processing: " << File << ".\n"; });<br>
+ ToolInvocation Invocation(std::move(CommandLine), Action, Files.get());<br>
+ Invocation.setDiagnosticConsumer(DiagConsumer);<br>
+ for (const auto &MappedFile : MappedFileContents) {<br>
+ Invocation.mapVirtualFile(MappedFile.first, MappedFile.second);<br>
+ }<br>
+ if (!Invocation.run()) {<br>
+ // FIXME: Diagnostics should be used instead.<br>
+ llvm::errs() << "Error while processing " << File << ".\n";<br>
+ ProcessingFailed = true;<br>
+ }<br>
}<br>
}<br>
return ProcessingFailed ? 1 : 0;<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br>
</div></div>