Re: [PATCH] D23618: [LibTooling] Allow compilation database to explicitly specify compilation database file and source root

[Alexander Kornienko via cfe-commits]

alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

As discussed offline, since the functionality being added in this patch is only 
useful for tests, the related simplification of tests seems not worth the added 
complexity.


https://reviews.llvm.org/D23618



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D23618: [LibTooling] Allow compilation database to explicitly specify compilation database file and source root

[Zachary Turner via cfe-commits]

zturner updated this revision to Diff 68404.
zturner added a comment.

Added full context diff.


https://reviews.llvm.org/D23618

Files:
  include/clang/Tooling/CompilationDatabase.h
  include/clang/Tooling/JSONCompilationDatabase.h
  lib/Tooling/CommonOptionsParser.cpp
  lib/Tooling/CompilationDatabase.cpp
  lib/Tooling/JSONCompilationDatabase.cpp
  lib/Tooling/Tooling.cpp
  tools/libclang/CXCompilationDatabase.cpp

Index: tools/libclang/CXCompilationDatabase.cpp
===
--- tools/libclang/CXCompilationDatabase.cpp
+++ tools/libclang/CXCompilationDatabase.cpp
@@ -17,7 +17,7 @@
   CXCompilationDatabase_Error Err = CXCompilationDatabase_NoError;
 
   std::unique_ptr db =
-  CompilationDatabase::loadFromDirectory(BuildDir, ErrorMsg);
+  CompilationDatabase::loadFromDirectory(BuildDir, StringRef(), ErrorMsg);
 
   if (!db) {
 fprintf(stderr, "LIBCLANG TOOLING ERROR: %s\n", ErrorMsg.c_str());
Index: lib/Tooling/Tooling.cpp
===
--- lib/Tooling/Tooling.cpp
+++ lib/Tooling/Tooling.cpp
@@ -407,10 +407,12 @@
   // difference for example on network filesystems, where symlinks might be
   // switched during runtime of the tool. Fixing this depends on having a
   // file system abstraction that allows openat() style interactions.
-  if (OverlayFileSystem->setCurrentWorkingDirectory(
-  CompileCommand.Directory))
-llvm::report_fatal_error("Cannot chdir into \"" +
- Twine(CompileCommand.Directory) + "\n!");
+  if (!CompileCommand.Directory.empty()) {
+if (OverlayFileSystem->setCurrentWorkingDirectory(
+CompileCommand.Directory))
+  llvm::report_fatal_error("Cannot chdir into \"" +
+   Twine(CompileCommand.Directory) + "\n!");
+  }
 
   // Now fill the in-memory VFS with the relative file mappings so it will
   // have the correct relative paths. We never remove mappings but that
Index: lib/Tooling/JSONCompilationDatabase.cpp
===
--- lib/Tooling/JSONCompilationDatabase.cpp
+++ lib/Tooling/JSONCompilationDatabase.cpp
@@ -16,10 +16,12 @@
 #include "clang/Tooling/CompilationDatabasePluginRegistry.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/Triple.h"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/StringSaver.h"
+#include 
 #include 
 
 namespace clang {
@@ -137,11 +139,18 @@
 
 class JSONCompilationDatabasePlugin : public CompilationDatabasePlugin {
   std::unique_ptr
-  loadFromDirectory(StringRef Directory, std::string ) override {
+  loadFromDirectory(StringRef Directory, StringRef SourceRoot,
+std::string ) override {
 SmallString<1024> JSONDatabasePath(Directory);
 llvm::sys::path::append(JSONDatabasePath, "compile_commands.json");
+return loadFromFile(JSONDatabasePath, SourceRoot, ErrorMessage);
+  }
+
+  std::unique_ptr
+  loadFromFile(StringRef File, StringRef SourceRoot,
+   std::string ) override {
 std::unique_ptr Database(
-JSONCompilationDatabase::loadFromFile(JSONDatabasePath, ErrorMessage));
+JSONCompilationDatabase::loadFromFile(File, SourceRoot, ErrorMessage));
 if (!Database)
   return nullptr;
 return Database;
@@ -160,16 +169,16 @@
 volatile int JSONAnchorSource = 0;
 
 std::unique_ptr
-JSONCompilationDatabase::loadFromFile(StringRef FilePath,
+JSONCompilationDatabase::loadFromFile(StringRef FilePath, StringRef SourceRoot,
   std::string ) {
   llvm::ErrorOr DatabaseBuffer =
   llvm::MemoryBuffer::getFile(FilePath);
   if (std::error_code Result = DatabaseBuffer.getError()) {
 ErrorMessage = "Error while opening JSON database: " + Result.message();
 return nullptr;
   }
   std::unique_ptr Database(
-  new JSONCompilationDatabase(std::move(*DatabaseBuffer)));
+  new JSONCompilationDatabase(SourceRoot, std::move(*DatabaseBuffer)));
   if (!Database->parse(ErrorMessage))
 return nullptr;
   return Database;
@@ -181,7 +190,7 @@
   std::unique_ptr DatabaseBuffer(
   llvm::MemoryBuffer::getMemBuffer(DatabaseString));
   std::unique_ptr Database(
-  new JSONCompilationDatabase(std::move(DatabaseBuffer)));
+  new JSONCompilationDatabase("", std::move(DatabaseBuffer)));
   if (!Database->parse(ErrorMessage))
 return nullptr;
   return Database;
@@ -245,12 +254,13 @@
 ArrayRef CommandsRef,
 std::vector ) const {
   for (int I = 0, E = CommandsRef.size(); I != E; ++I) {
-SmallString<8> DirectoryStorage;
+SmallString<128> DirectoryStorage;
 SmallString<32> FilenameStorage;
+getDirectoryForCommand(*std::get<0>(CommandsRef[I]), DirectoryStorage);
 

Re: [PATCH] D23618: [LibTooling] Allow compilation database to explicitly specify compilation database file and source root

[Zachary Turner via cfe-commits]

Strange, I thought i did. Will re upload
On Wed, Aug 17, 2016 at 12:14 PM Alexander Kornienko 
wrote:

> alexfh added a comment.
>
> Full context diffs, please (
> http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
> ).
>
>
> https://reviews.llvm.org/D23618
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D23618: [LibTooling] Allow compilation database to explicitly specify compilation database file and source root

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

Full context diffs, please 
(http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface).


https://reviews.llvm.org/D23618



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D23618: [LibTooling] Allow compilation database to explicitly specify compilation database file and source root

[Zachary Turner via cfe-commits]

zturner created this revision.
zturner added reviewers: djasper, alexfh, klimek.
zturner added a subscriber: cfe-commits.
Herald added a subscriber: klimek.

Allow explicit specification of a compilation database file and source root.

While trying to create a compilation database test for D23455, I ran into the 
problem that compilation database tests require shell.  Shell tests are 
inefficient and cumbersome for a number of reasons.  For one it takes a long 
time reading the test to understand what it does.  Secondly it makes it harder 
to modify the test.  Third creating many processes as is common with a shell 
test is slow on Windows.

I decided to solve this by introducing a new command line parameter and 
slightly enhancing the functionality of an existing one, detailed below:

1. There was no way to specify an arbitrary compilation database.  At best you 
could specify the directory of a compilation database, but it would still be 
forced to look for `compile_commands.json`.  This is an inflexible design when 
you want to have multiple tests which each use their own compilation database.  
You could put them in different folders and call each one 
`compile_commands.json`, but that leads to many different directories with only 
one file, which is kind of silly.

The `-p` option let you specify a build path, which would be the folder where a 
compilation database existed, but it did not let you specify a *specific file* 
to use as a compilation database.  I improved expanded this behavior of this 
option to check if the specified path is a regular file or a directory.  If it 
is a regular file it doesn't search for a compilation database, and instead 
just uses the file you specify.  The nice thing about this is that now we can 
put many compilation databases for various tests in the same directory, and 
have the run line of the test reference the exact compilation database needed 
for the test.

2. Instead of a "real" compilation database, the test consisted of a template.  
And the test used shell in order to invoke `sed` to modify the template so that 
the files and directories would be correct.  So the test would output the 
modified template into the CMake output directory, and then it had to use more 
shell commands to copy the source files over to the CMake output directory so 
that they would be in the right hierarchy in order to be found by the relative 
paths written to the compilation database.

In order to remove the dependency on shell commands, we need a way to reference 
the source files in their original location in the source tree, and not require 
them to be copied to the output tree.  To do this I introduced a new command 
line option `-r` that lets you specify the root at which relative paths are 
resolved against.  

These two things combined eliminate the need to modify the compilation database 
with `sed` and write it to the output folder because we can write *actual* 
source code on disk and check it in for the tests, and the compilation database 
can refer to these files using actual relative paths.  Then because of #1, the 
run line can point directly to the compilation database in question.

I re-wrote the compilation database test to use this new method, and it now no 
longer depends on shell.  I will upload the re-written test in a separate patch 
since I can't do cross-repo diffs.

After applying this patch, the compilation database test can be written using a 
single run line.  A patch that does exactly that is in D23533

https://reviews.llvm.org/D23618

Files:
  include/clang/Tooling/CompilationDatabase.h
  include/clang/Tooling/JSONCompilationDatabase.h
  lib/Tooling/CommonOptionsParser.cpp
  lib/Tooling/CompilationDatabase.cpp
  lib/Tooling/JSONCompilationDatabase.cpp
  lib/Tooling/Tooling.cpp

Index: lib/Tooling/Tooling.cpp
===
--- lib/Tooling/Tooling.cpp
+++ lib/Tooling/Tooling.cpp
@@ -407,10 +407,12 @@
   // difference for example on network filesystems, where symlinks might be
   // switched during runtime of the tool. Fixing this depends on having a
   // file system abstraction that allows openat() style interactions.
-  if (OverlayFileSystem->setCurrentWorkingDirectory(
-  CompileCommand.Directory))
-llvm::report_fatal_error("Cannot chdir into \"" +
- Twine(CompileCommand.Directory) + "\n!");
+  if (!CompileCommand.Directory.empty()) {
+if (OverlayFileSystem->setCurrentWorkingDirectory(
+CompileCommand.Directory))
+  llvm::report_fatal_error("Cannot chdir into \"" +
+   Twine(CompileCommand.Directory) + "\n!");
+  }
 
   // Now fill the in-memory VFS with the relative file mappings so it will
   // have the correct relative paths. We never remove mappings but that
Index: lib/Tooling/JSONCompilationDatabase.cpp