[PATCH] D41911: [clangd] Include scanner that finds compile commands for headers.
sammccall marked 2 inline comments as done. sammccall added a comment. Context here, this patch is over a year old and predates the work on heuristically picking a compile command from another file in the CDB. I don't think anyone has concrete plans to revive this, the heuristics work well enough and often enough. I should probably have abandoned this patch, sorry for the time lost in detailed comments. However there's certainly some value in indexing the include graph and/or determining main-file/header pairs. e.g. to improve switchsourceheader, to enable cross-file refactorings like "out-line implementation". And maybe to improve compile command accuracy too. This patch was pretty narrowly tailored at getting compile commands quickly, if we developed this approach it should probably be more general purpose, part of the index, and maybe take symbol declaration/definition into account rather than just #includes. Comment at: clangd/IncludeScanner.cpp:131 + IgnoringDiagConsumer IgnoreDiags; + // XXX the VFS working directory is global state, this is unsafe. + Cmd.VFS->setCurrentWorkingDirectory(Cmd.Directory); klimek wrote: > I assume that's a note to you that you want to change this? This has since been fixed, it's no longer global state :-) Comment at: clangd/IncludeScanner.cpp:159 + // When reusing flags, we add an explicit `-x cpp` to override the extension. + // TODO: copying CompilerInvocation would avoid this, and is more robust. + if (std::find(Cmd.CommandLine.begin(), Cmd.CommandLine.end(), "-x") == klimek wrote: > Wondering why we can't use one of the tooling abstractions here... As far as I can tell, the only abstraction Tooling offers here is CompileCommand, the only way to robustly manipulate it is through the Driver (the semantics here are too subtle for brute-force string manipulation). InterpolatingCompilationDatabase does this, but doesn't expose the primitives publicly. It could if there was a need. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D41911/new/ https://reviews.llvm.org/D41911 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41911: [clangd] Include scanner that finds compile commands for headers.
klimek added inline comments.
Comment at: clangd/IncludeScanner.cpp:75
+ bool WasEmpty = Queue.empty();
+ for (const auto : Cmds) {
+QueueEntry E(Cmd, VFS);
Usually I'd try to not lock a loop, as a large number of compile commands now
blocks other threads for a bit. Fairness might not matter here, though.
Comment at: clangd/IncludeScanner.cpp:97
+auto = Queue.front();
+Lock.unlock();
+process(std::move(Entry));
I'd find it a bit more idiomatic to have two unique_locked blocks instead
(easier to pull out a function when things grow etc), but I see it's annoying
to have to default-construct an entry in that case :(
Given that I'd probably pull that into a function, but then we need to
communicate Done. Sigh.
Comment at: clangd/IncludeScanner.cpp:100
+Lock.lock();
+Queue.pop_front();
+if (Queue.empty()) {
I'd try to write the code so multiple consumer threads would work - given that
process() can't fail, why not pop in the locked session above?
Comment at: clangd/IncludeScanner.cpp:131
+ IgnoringDiagConsumer IgnoreDiags;
+ // XXX the VFS working directory is global state, this is unsafe.
+ Cmd.VFS->setCurrentWorkingDirectory(Cmd.Directory);
I assume that's a note to you that you want to change this?
Comment at: clangd/IncludeScanner.cpp:159
+ // When reusing flags, we add an explicit `-x cpp` to override the extension.
+ // TODO: copying CompilerInvocation would avoid this, and is more robust.
+ if (std::find(Cmd.CommandLine.begin(), Cmd.CommandLine.end(), "-x") ==
Wondering why we can't use one of the tooling abstractions here...
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D41911/new/
https://reviews.llvm.org/D41911
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41911: [clangd] Include scanner that finds compile commands for headers.
nridge added a comment. Herald added subscribers: jfb, kadircet, arphaman, jkorous, MaskRay. Herald added a project: clang. Are there are plans to move forward with this sort of an include scanning approach? Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D41911/new/ https://reviews.llvm.org/D41911 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41911: [clangd] Include scanner that finds compile commands for headers.
sammccall created this revision.
sammccall added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, mgorny, klimek.
Not enabled because we need a threadsafe way to change VFS working directories.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D41911
Files:
clangd/CMakeLists.txt
clangd/Compiler.cpp
clangd/IncludeScanner.cpp
clangd/IncludeScanner.h
unittests/clangd/CMakeLists.txt
unittests/clangd/IncludeScannerTests.cpp
Index: unittests/clangd/IncludeScannerTests.cpp
===
--- /dev/null
+++ unittests/clangd/IncludeScannerTests.cpp
@@ -0,0 +1,61 @@
+//===-- IncludeScannerTests.cpp --*- C++ -*---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "IncludeScanner.h"
+#include "TestFS.h"
+#include "llvm/ADT/StringMap.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include "Logger.h"
+
+namespace clang {
+namespace tooling {
+void PrintTo(const llvm::Optional , std::ostream *OS) {
+ if (S)
+*OS << llvm::join(S->CommandLine, " ");
+ else
+*OS << "";
+}
+} // namespace tooling
+namespace clangd {
+namespace {
+
+MATCHER_P2(HasCmd, Args, Filename, "") {
+ return arg &&
+ arg->CommandLine == Args &&
+ arg->Filename == Filename;
+}
+
+testing::Matcher
+Cmd(std::vector Args) {
+ return HasCmd(Args, Args.back());
+}
+
+TEST(IncludeScanner, FindsFiles) {
+ llvm::StringMap Files;
+ llvm::StringRef One = getVirtualTestFilePath("one.cc"),
+ Two = getVirtualTestFilePath("dir/two.h"),
+ Three = getVirtualTestFilePath("dir/three.h");
+ Files[One] = R"cpp(#include "dir/two.h")cpp";
+ Files[Two] = R"cpp(#include "three.h")cpp";
+ Files[Three] = "";
+ auto FS = buildTestFS(Files);
+
+ IncludeScanner Scanner;
+ Scanner.enqueue({
+ {".", One, {"clang", "-DX", One}, ""},
+ }, FS);
+ Scanner.wait();
+ EXPECT_THAT(Scanner.lookup(Two), Cmd({"clang", "-DX", "-x", "c++", Two}));
+ EXPECT_THAT(Scanner.lookup(Three), Cmd({"clang", "-DX", "-x", "c++", Three}));
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -16,6 +16,7 @@
ContextTests.cpp
FileIndexTests.cpp
FuzzyMatchTests.cpp
+ IncludeScannerTests.cpp
IndexTests.cpp
JSONExprTests.cpp
TestFS.cpp
Index: clangd/IncludeScanner.h
===
--- /dev/null
+++ clangd/IncludeScanner.h
@@ -0,0 +1,94 @@
+//===--- IncludeScanner.h - Infer compile commands for headers --*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===-===//
+// Typical compilation databases don't list commands for headers. But headers
+// can be compiled with the same flags as the files that include them.
+// So when we find a database, we scan through the commands it do
+// preprocessor to find #included files that command is valid for.
+//===-===//
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INCLUDESCANNER_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INCLUDESCANNER_H
+#include "Context.h"
+#include "Path.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringSet.h"
+#include
+#include
+#include
+#include
+#include
+#include
+
+namespace clang {
+class PCHContainerOperations;
+namespace vfs {
+class FileSystem;
+}
+namespace tooling {
+struct CompileCommand;
+} // namespace tooling
+namespace clangd {
+
+// IncludeScanner runs a background thread that scans files for which compile
+// commands are known, recording the headers for which that command is valid.
+//
+// It supports lookup by header name. As scanning doesn't block, it's always
+// possible we won't have scanned the right compile command yet.
+// This means the compilation database should also have heuristics for headers.
+//
+// This class is threadsafe.
+class IncludeScanner {
+public:
+ IncludeScanner();
+
+ // The destructor may wait for the current file to finish scanning.
+ ~IncludeScanner();
+
+ // If we've scanned some file that #includes Header, return the inferred
+ // compile command. This will have the same flags, but the filename replaced.
+ llvm::Optional lookup(PathRef Header) const;
+
+ // Adds compile commands to scan. Files we've already scanned will be ignored.
+ void enqueue(std::vector Cmds,
+
