[PATCH] D82964: [clangd] Config: loading and caching config from disk.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG8bd000a65fe4: [clangd] Config: loading and caching config
from disk. (authored by sammccall).
Changed prior to commit:
https://reviews.llvm.org/D82964?vs=275384=275484#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82964/new/
https://reviews.llvm.org/D82964
Files:
clang-tools-extra/clangd/CMakeLists.txt
clang-tools-extra/clangd/ConfigProvider.cpp
clang-tools-extra/clangd/ConfigProvider.h
clang-tools-extra/clangd/unittests/CMakeLists.txt
clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
clang-tools-extra/clangd/unittests/TestFS.cpp
clang-tools-extra/clangd/unittests/TestFS.h
Index: clang-tools-extra/clangd/unittests/TestFS.h
===
--- clang-tools-extra/clangd/unittests/TestFS.h
+++ clang-tools-extra/clangd/unittests/TestFS.h
@@ -69,7 +69,8 @@
const char *testRoot();
// Returns a suitable absolute path for this OS.
-std::string testPath(PathRef File);
+std::string testPath(PathRef File,
+ llvm::sys::path::Style = llvm::sys::path::Style::native);
// unittest: is a scheme that refers to files relative to testRoot()
// This anchor is used to force the linker to link in the generated object file
Index: clang-tools-extra/clangd/unittests/TestFS.cpp
===
--- clang-tools-extra/clangd/unittests/TestFS.cpp
+++ clang-tools-extra/clangd/unittests/TestFS.cpp
@@ -79,13 +79,13 @@
#endif
}
-std::string testPath(PathRef File) {
+std::string testPath(PathRef File, llvm::sys::path::Style Style) {
assert(llvm::sys::path::is_relative(File) && "FileName should be relative");
llvm::SmallString<32> NativeFile = File;
- llvm::sys::path::native(NativeFile);
+ llvm::sys::path::native(NativeFile, Style);
llvm::SmallString<32> Path;
- llvm::sys::path::append(Path, testRoot(), NativeFile);
+ llvm::sys::path::append(Path, Style, testRoot(), NativeFile);
return std::string(Path.str());
}
Index: clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
@@ -0,0 +1,156 @@
+//===-- ConfigProviderTests.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Config.h"
+#include "ConfigProvider.h"
+#include "ConfigTesting.h"
+#include "TestFS.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include "llvm/Support/SourceMgr.h"
+#include
+
+namespace clang {
+namespace clangd {
+namespace config {
+namespace {
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+
+// Provider that appends an arg to compile flags.
+// The arg is prefix, where N is the times getFragments() was called.
+// It also yields a diagnostic each time it's called.
+class FakeProvider : public Provider {
+ std::string Prefix;
+ mutable std::atomic Index = {0};
+
+ std::vector
+ getFragments(const Params &, DiagnosticCallback DC) const override {
+DC(llvm::SMDiagnostic("", llvm::SourceMgr::DK_Error, Prefix));
+CompiledFragment F =
+[Arg(Prefix + std::to_string(++Index))](const Params , Config ) {
+ C.CompileFlags.Edits.push_back(
+ [Arg](std::vector ) { Argv.push_back(Arg); });
+ return true;
+};
+return {F};
+ }
+
+public:
+ FakeProvider(llvm::StringRef Prefix) : Prefix(Prefix) {}
+};
+
+std::vector getAddedArgs(Config ) {
+ std::vector Argv;
+ for (auto : C.CompileFlags.Edits)
+Edit(Argv);
+ return Argv;
+}
+
+// The provider from combine() should invoke its providers in order, and not
+// cache their results.
+TEST(ProviderTest, Combine) {
+ CapturedDiags Diags;
+ std::vector> Providers;
+ Providers.push_back(std::make_unique("foo"));
+ Providers.push_back(std::make_unique("bar"));
+ auto Combined = Provider::combine(std::move(Providers));
+ Config Cfg = Combined->getConfig(Params(), Diags.callback());
+ EXPECT_THAT(Diags.Diagnostics,
+ ElementsAre(DiagMessage("foo"), DiagMessage("bar")));
+ EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo1", "bar1"));
+ Diags.Diagnostics.clear();
+
+ Cfg = Combined->getConfig(Params(), Diags.callback());
+ EXPECT_THAT(Diags.Diagnostics,
+ ElementsAre(DiagMessage("foo"), DiagMessage("bar")));
+ EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo2", "bar2"));
+}
+
+const char *AddFooWithErr = R"yaml(
+CompileFlags:
+ Add: foo
+ Unknown: 42
+)yaml";
+
+const char *AddBarBaz = R"yaml(
+CompileFlags:
+ Add: bar
+---
+CompileFlags:
+
[PATCH] D82964: [clangd] Config: loading and caching config from disk.
kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82964/new/ https://reviews.llvm.org/D82964 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82964: [clangd] Config: loading and caching config from disk.
sammccall added inline comments. Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:39 +auto Buf = FS.getBufferForFile(Path); +// If stat() succeeds but we failed to read, retry once and cache failure. +if (!Buf) kadircet wrote: > why do we want to cache failure case for missing files ? > > If it is truly missing and we didn't race with another process (likely > git-checkout), the next stat will fail and we'll return a no-op fragment. > If it was a race, we want to pick it up on the next run (and reading twice > might not have enough of a delay in between), so why not just cache with a > sentinel key in here too? Yeah I was worried about a file we can stat but chronically can't read (e.g. because permissions are wrong). But this is probably not worth worrying about. Changed no not cache as you suggest. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82964/new/ https://reviews.llvm.org/D82964 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82964: [clangd] Config: loading and caching config from disk.
sammccall updated this revision to Diff 275384.
sammccall marked 2 inline comments as done.
sammccall added a comment.
Don't cache failed opens.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82964/new/
https://reviews.llvm.org/D82964
Files:
clang-tools-extra/clangd/CMakeLists.txt
clang-tools-extra/clangd/ConfigProvider.cpp
clang-tools-extra/clangd/ConfigProvider.h
clang-tools-extra/clangd/unittests/CMakeLists.txt
clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
clang-tools-extra/clangd/unittests/TestFS.cpp
clang-tools-extra/clangd/unittests/TestFS.h
Index: clang-tools-extra/clangd/unittests/TestFS.h
===
--- clang-tools-extra/clangd/unittests/TestFS.h
+++ clang-tools-extra/clangd/unittests/TestFS.h
@@ -69,7 +69,8 @@
const char *testRoot();
// Returns a suitable absolute path for this OS.
-std::string testPath(PathRef File);
+std::string testPath(PathRef File,
+ llvm::sys::path::Style = llvm::sys::path::Style::native);
// unittest: is a scheme that refers to files relative to testRoot()
// This anchor is used to force the linker to link in the generated object file
Index: clang-tools-extra/clangd/unittests/TestFS.cpp
===
--- clang-tools-extra/clangd/unittests/TestFS.cpp
+++ clang-tools-extra/clangd/unittests/TestFS.cpp
@@ -79,13 +79,13 @@
#endif
}
-std::string testPath(PathRef File) {
+std::string testPath(PathRef File, llvm::sys::path::Style Style) {
assert(llvm::sys::path::is_relative(File) && "FileName should be relative");
llvm::SmallString<32> NativeFile = File;
- llvm::sys::path::native(NativeFile);
+ llvm::sys::path::native(NativeFile, Style);
llvm::SmallString<32> Path;
- llvm::sys::path::append(Path, testRoot(), NativeFile);
+ llvm::sys::path::append(Path, Style, testRoot(), NativeFile);
return std::string(Path.str());
}
Index: clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
@@ -0,0 +1,156 @@
+//===-- ConfigProviderTests.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Config.h"
+#include "ConfigProvider.h"
+#include "ConfigTesting.h"
+#include "TestFS.h"
+#include "llvm/Support/SourceMgr.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include
+
+namespace clang {
+namespace clangd {
+namespace config {
+namespace {
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+
+// Provider that appends an arg to compile flags.
+// The arg is prefix, where N is the times getFragments() was called.
+// It also yields a diagnostic each time it's called.
+class FakeProvider : public Provider {
+ std::string Prefix;
+ mutable std::atomic Index = {0};
+
+ std::vector
+ getFragments(const Params &, DiagnosticCallback DC) const override {
+DC(llvm::SMDiagnostic("", llvm::SourceMgr::DK_Error, Prefix));
+CompiledFragment F =
+[Arg(Prefix + std::to_string(++Index))](const Params , Config ) {
+ C.CompileFlags.Edits.push_back(
+ [Arg](std::vector ) { Argv.push_back(Arg); });
+ return true;
+};
+return {F};
+ }
+
+public:
+ FakeProvider(llvm::StringRef Prefix) : Prefix(Prefix) {}
+};
+
+std::vector getAddedArgs(Config ) {
+ std::vector Argv;
+ for (auto : C.CompileFlags.Edits)
+Edit(Argv);
+ return Argv;
+}
+
+// The provider from combine() should invoke its providers in order, and not
+// cache their results.
+TEST(ProviderTest, Combine) {
+ CapturedDiags Diags;
+ std::vector> Providers;
+ Providers.push_back(std::make_unique("foo"));
+ Providers.push_back(std::make_unique("bar"));
+ auto Combined = Provider::combine(std::move(Providers));
+ Config Cfg = Combined->getConfig(Params(), Diags.callback());
+ EXPECT_THAT(Diags.Diagnostics,
+ ElementsAre(DiagMessage("foo"), DiagMessage("bar")));
+ EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo1", "bar1"));
+ Diags.Diagnostics.clear();
+
+ Cfg = Combined->getConfig(Params(), Diags.callback());
+ EXPECT_THAT(Diags.Diagnostics,
+ ElementsAre(DiagMessage("foo"), DiagMessage("bar")));
+ EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo2", "bar2"));
+}
+
+const char *AddFooWithErr = R"yaml(
+CompileFlags:
+ Add: foo
+ Unknown: 42
+)yaml";
+
+const char *AddBarBaz = R"yaml(
+CompileFlags:
+ Add: bar
+---
+CompileFlags:
+ Add: baz
+)yaml";
+
+TEST(ProviderTest, FromYAMLFile) {
+ MockFS FS;
+ FS.Files["foo.yaml"] = AddFooWithErr;
+
+
[PATCH] D82964: [clangd] Config: loading and caching config from disk.
kadircet added inline comments. Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:39 +auto Buf = FS.getBufferForFile(Path); +// If stat() succeeds but we failed to read, retry once and cache failure. +if (!Buf) why do we want to cache failure case for missing files ? If it is truly missing and we didn't race with another process (likely git-checkout), the next stat will fail and we'll return a no-op fragment. If it was a race, we want to pick it up on the next run (and reading twice might not have enough of a delay in between), so why not just cache with a sentinel key in here too? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82964/new/ https://reviews.llvm.org/D82964 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82964: [clangd] Config: loading and caching config from disk.
sammccall updated this revision to Diff 275165.
sammccall added a comment.
Try to get the tests to pass on windows.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82964/new/
https://reviews.llvm.org/D82964
Files:
clang-tools-extra/clangd/CMakeLists.txt
clang-tools-extra/clangd/ConfigProvider.cpp
clang-tools-extra/clangd/ConfigProvider.h
clang-tools-extra/clangd/unittests/CMakeLists.txt
clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
clang-tools-extra/clangd/unittests/TestFS.cpp
clang-tools-extra/clangd/unittests/TestFS.h
Index: clang-tools-extra/clangd/unittests/TestFS.h
===
--- clang-tools-extra/clangd/unittests/TestFS.h
+++ clang-tools-extra/clangd/unittests/TestFS.h
@@ -69,7 +69,8 @@
const char *testRoot();
// Returns a suitable absolute path for this OS.
-std::string testPath(PathRef File);
+std::string testPath(PathRef File,
+ llvm::sys::path::Style = llvm::sys::path::Style::native);
// unittest: is a scheme that refers to files relative to testRoot()
// This anchor is used to force the linker to link in the generated object file
Index: clang-tools-extra/clangd/unittests/TestFS.cpp
===
--- clang-tools-extra/clangd/unittests/TestFS.cpp
+++ clang-tools-extra/clangd/unittests/TestFS.cpp
@@ -79,13 +79,13 @@
#endif
}
-std::string testPath(PathRef File) {
+std::string testPath(PathRef File, llvm::sys::path::Style Style) {
assert(llvm::sys::path::is_relative(File) && "FileName should be relative");
llvm::SmallString<32> NativeFile = File;
- llvm::sys::path::native(NativeFile);
+ llvm::sys::path::native(NativeFile, Style);
llvm::SmallString<32> Path;
- llvm::sys::path::append(Path, testRoot(), NativeFile);
+ llvm::sys::path::append(Path, Style, testRoot(), NativeFile);
return std::string(Path.str());
}
Index: clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
@@ -0,0 +1,156 @@
+//===-- ConfigProviderTests.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Config.h"
+#include "ConfigProvider.h"
+#include "ConfigTesting.h"
+#include "TestFS.h"
+#include "llvm/Support/SourceMgr.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include
+
+namespace clang {
+namespace clangd {
+namespace config {
+namespace {
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+
+// Provider that appends an arg to compile flags.
+// The arg is prefix, where N is the times getFragments() was called.
+// It also yields a diagnostic each time it's called.
+class FakeProvider : public Provider {
+ std::string Prefix;
+ mutable std::atomic Index = {0};
+
+ std::vector
+ getFragments(const Params &, DiagnosticCallback DC) const override {
+DC(llvm::SMDiagnostic("", llvm::SourceMgr::DK_Error, Prefix));
+CompiledFragment F =
+[Arg(Prefix + std::to_string(++Index))](const Params , Config ) {
+ C.CompileFlags.Edits.push_back(
+ [Arg](std::vector ) { Argv.push_back(Arg); });
+ return true;
+};
+return {F};
+ }
+
+public:
+ FakeProvider(llvm::StringRef Prefix) : Prefix(Prefix) {}
+};
+
+std::vector getAddedArgs(Config ) {
+ std::vector Argv;
+ for (auto : C.CompileFlags.Edits)
+Edit(Argv);
+ return Argv;
+}
+
+// The provider from combine() should invoke its providers in order, and not
+// cache their results.
+TEST(ProviderTest, Combine) {
+ CapturedDiags Diags;
+ std::vector> Providers;
+ Providers.push_back(std::make_unique("foo"));
+ Providers.push_back(std::make_unique("bar"));
+ auto Combined = Provider::combine(std::move(Providers));
+ Config Cfg = Combined->getConfig(Params(), Diags.callback());
+ EXPECT_THAT(Diags.Diagnostics,
+ ElementsAre(DiagMessage("foo"), DiagMessage("bar")));
+ EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo1", "bar1"));
+ Diags.Diagnostics.clear();
+
+ Cfg = Combined->getConfig(Params(), Diags.callback());
+ EXPECT_THAT(Diags.Diagnostics,
+ ElementsAre(DiagMessage("foo"), DiagMessage("bar")));
+ EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo2", "bar2"));
+}
+
+const char *AddFooWithErr = R"yaml(
+CompileFlags:
+ Add: foo
+ Unknown: 42
+)yaml";
+
+const char *AddBarBaz = R"yaml(
+CompileFlags:
+ Add: bar
+---
+CompileFlags:
+ Add: baz
+)yaml";
+
+TEST(ProviderTest, FromYAMLFile) {
+ MockFS FS;
+ FS.Files["foo.yaml"] = AddFooWithErr;
+
+ CapturedDiags Diags;
+ auto P =
[PATCH] D82964: [clangd] Config: loading and caching config from disk.
sammccall updated this revision to Diff 275146.
sammccall marked 5 inline comments as done.
sammccall added a comment.
address comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82964/new/
https://reviews.llvm.org/D82964
Files:
clang-tools-extra/clangd/CMakeLists.txt
clang-tools-extra/clangd/ConfigProvider.cpp
clang-tools-extra/clangd/ConfigProvider.h
clang-tools-extra/clangd/unittests/CMakeLists.txt
clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
Index: clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
@@ -0,0 +1,154 @@
+//===-- ConfigProviderTests.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Config.h"
+#include "ConfigProvider.h"
+#include "ConfigTesting.h"
+#include "TestFS.h"
+#include "llvm/Support/SourceMgr.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include
+
+namespace clang {
+namespace clangd {
+namespace config {
+namespace {
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+
+// Provider that appends an arg to compile flags.
+// The arg is prefix, where N is the times getFragments() was called.
+// It also yields a diagnostic each time it's called.
+class FakeProvider : public Provider {
+ std::string Prefix;
+ mutable std::atomic Index = {0};
+
+ std::vector
+ getFragments(const Params &, DiagnosticCallback DC) const override {
+DC(llvm::SMDiagnostic("", llvm::SourceMgr::DK_Error, Prefix));
+CompiledFragment F =
+[Arg(Prefix + std::to_string(++Index))](const Params , Config ) {
+ C.CompileFlags.Edits.push_back(
+ [Arg](std::vector ) { Argv.push_back(Arg); });
+ return true;
+};
+return {F};
+ }
+
+public:
+ FakeProvider(llvm::StringRef Prefix) : Prefix(Prefix) {}
+};
+
+std::vector getAddedArgs(Config ) {
+ std::vector Argv;
+ for (auto : C.CompileFlags.Edits)
+Edit(Argv);
+ return Argv;
+}
+
+// The provider from combine() should invoke its providers in order, and not
+// cache their results.
+TEST(ProviderTest, Combine) {
+ CapturedDiags Diags;
+ std::vector> Providers;
+ Providers.push_back(std::make_unique("foo"));
+ Providers.push_back(std::make_unique("bar"));
+ auto Combined = Provider::combine(std::move(Providers));
+ Config Cfg = Combined->getConfig(Params(), Diags.callback());
+ EXPECT_THAT(Diags.Diagnostics,
+ ElementsAre(DiagMessage("foo"), DiagMessage("bar")));
+ EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo1", "bar1"));
+ Diags.Diagnostics.clear();
+
+ Cfg = Combined->getConfig(Params(), Diags.callback());
+ EXPECT_THAT(Diags.Diagnostics,
+ ElementsAre(DiagMessage("foo"), DiagMessage("bar")));
+ EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo2", "bar2"));
+}
+
+const char *AddFooWithErr = R"yaml(
+CompileFlags:
+ Add: foo
+ Unknown: 42
+)yaml";
+
+const char *AddBarBaz = R"yaml(
+CompileFlags:
+ Add: bar
+---
+CompileFlags:
+ Add: baz
+)yaml";
+
+TEST(ProviderTest, FromYAMLFile) {
+ MockFS FS;
+ FS.Files["foo.yaml"] = AddFooWithErr;
+
+ CapturedDiags Diags;
+ auto P = Provider::fromYAMLFile(testPath("foo.yaml"), FS);
+ auto Cfg = P->getConfig(Params(), Diags.callback());
+ EXPECT_THAT(Diags.Diagnostics,
+ ElementsAre(DiagMessage("Unknown CompileFlags key Unknown")));
+ EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo"));
+ Diags.Diagnostics.clear();
+
+ Cfg = P->getConfig(Params(), Diags.callback());
+ EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "Cached, not re-parsed";
+ EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo"));
+
+ FS.Files["foo.yaml"] = AddBarBaz;
+ Cfg = P->getConfig(Params(), Diags.callback());
+ EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "New config, no errors";
+ EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("bar", "baz"));
+
+ FS.Files.erase("foo.yaml");
+ Cfg = P->getConfig(Params(), Diags.callback());
+ EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "Missing file is not an error";
+ EXPECT_THAT(getAddedArgs(Cfg), IsEmpty());
+}
+
+TEST(ProviderTest, FromAncestorRelativeYAMLFiles) {
+ MockFS FS;
+ FS.Files["a/b/c/foo.yaml"] = AddBarBaz;
+ FS.Files["a/foo.yaml"] = AddFooWithErr;
+
+ std::string ABCPath = testPath("a/b/c/d/test.cc");
+ Params ABCParams;
+ ABCParams.Path = ABCPath;
+ std::string APath = testPath("a/b/e/f/test.cc");
+ Params AParams;
+ AParams.Path = APath;
+
+ CapturedDiags Diags;
+ auto P = Provider::fromAncestorRelativeYAMLFiles("foo.yaml", FS);
+
+ auto Cfg = P->getConfig(Params(), Diags.callback());
+
[PATCH] D82964: [clangd] Config: loading and caching config from disk.
sammccall marked 15 inline comments as done.
sammccall added inline comments.
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:39
+std::vector ) {
+assert(llvm::sys::path::is_absolute(Path));
+auto FS = TFS.view(/*CWD=*/llvm::None);
kadircet wrote:
> I believe this function is going to be hot, I would suggest putting a span
> here to track the latencies
Tracing isn't trivially cheap: that usually involves taking a mutex, writing to
a buffer, and I think flushing it to disk.
So we shouldn't trace things that are both hot and small, which I think this is.
I added a tracer to Provider::getConfig - OK to revisit if we need something
more fine-grained?
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:41
+auto FS = TFS.view(/*CWD=*/llvm::None);
+auto Stat = FS->status(Path);
+if (!Stat || !Stat->isRegularFile()) {
kadircet wrote:
> i suppose doing this IO on most of our features are OK, as they tend to
> search for .clang-format at least (even the ancestor traversal), but I wonder
> whether these will also be triggered for code completion and signature help,
> as they currently don't do any IO (apart from preamble deserialization, and
> maybe one patched include file).
>
> Not a concern for this patch, but something to keep in mind when integrating
> with the rest.
Yeah, this is a good point.
One possible design would be to have a "latency-sensitivity" setting, probably
on `Params`, and not bothering to stat the file if we're highly sensitive.
(i.e. always reuse the cached value unless the cache is uninitialized).
I've added this idea to the class comment.
Related: I think an abstraction like this, with some thought put into
invalidation policy, should be eventually used for `.clang-format`,
`.clang-tidy`, `compile_commands.json`, as we should be able to get fresh
results with a fairly tiny amount of IO.
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:57
+// For simplicity, don't cache the value in this case (use a bad key).
+if (Buf->get()->getBufferSize() != Size) {
+ Size = -1;
kadircet wrote:
> is this really necessary ? we are setting the cache key to old stat values
> anyway, so the next read shouldn't match neither the Size nor MTime.
I considered doing nothing here, or setting Size = Buf->size.
But there are *possible* scenarios where you go back to the old state (think
switching git branches) and then end up with a poisoned cache.
And this is cheap (simple, and also doesn't actually cause you to miss the
cache when you want to hit it)
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:67
+}
+llvm::copy(CachedValue, std::back_inserter(Out));
+ }
kadircet wrote:
> nit: I've found the nestedness a bit overwhelming, wdyt about putting this
> into a scope_exit function and using early returns above ?
scope_exit is a neat idea, but when I tried it I found the code hard to reason
about.
I extracted a function instead.
Unnesting this caused me to catch a case i'd missed: if stat succeeds but read
fails (once), I'd prefer to retry the read before caching the failure
forever... (again, the branch switching workflows)
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:120
+ I != E; ++I) {
+// Avoid weird non-substring cases like phantom "." components.
+if (I->end() < Parent.begin() && I->end() > Parent.end())
kadircet wrote:
> ummm, what ? is this a bug in path iterator that should be fixed rather than
> worked around?
No, it's documented that Component isn't necessarily part of the path.
The only case I could find of this actually happening is that "/foo/bar/"
iterates as "foo", "bar", ".", at least in some cases.
Extended the comment a bit.
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:181
+ for (const auto : getFragments(P, DC))
+Fragment(P, C);
+ return C;
kadircet wrote:
> nit: I think this looks a bit ... surprising
the fact that this is Fragment instead of Fragment(P, C)?
Yeah, this is an argument for adding an interface rather than using
std::function.
But I don't think it's a big deal or really actionable in this patch.
Comment at: clang-tools-extra/clangd/ConfigProvider.h:85
+ /// When parsing/compiling, the DiagnosticCallback is used to report errors.
+ /// Usual thread-safety guarantees apply: this function must be threadsafe.
+ virtual std::vector
kadircet wrote:
> first part of this sentence doesn't say much, maybe drop it?
I think just saying "this function must be threadsafe" is a bit confusing
because it's a const method, it's like saying "this function does not throw
exceptions" in llvm code.
But I shoud have made my point more explicit. Replaced with "Despite the need
[PATCH] D82964: [clangd] Config: loading and caching config from disk.
kadircet added a comment.
thanks looks really cool!
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:39
+std::vector ) {
+assert(llvm::sys::path::is_absolute(Path));
+auto FS = TFS.view(/*CWD=*/llvm::None);
I believe this function is going to be hot, I would suggest putting a span here
to track the latencies
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:41
+auto FS = TFS.view(/*CWD=*/llvm::None);
+auto Stat = FS->status(Path);
+if (!Stat || !Stat->isRegularFile()) {
i suppose doing this IO on most of our features are OK, as they tend to search
for .clang-format at least (even the ancestor traversal), but I wonder whether
these will also be triggered for code completion and signature help, as they
currently don't do any IO (apart from preamble deserialization, and maybe one
patched include file).
Not a concern for this patch, but something to keep in mind when integrating
with the rest.
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:57
+// For simplicity, don't cache the value in this case (use a bad key).
+if (Buf->get()->getBufferSize() != Size) {
+ Size = -1;
is this really necessary ? we are setting the cache key to old stat values
anyway, so the next read shouldn't match neither the Size nor MTime.
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:67
+}
+llvm::copy(CachedValue, std::back_inserter(Out));
+ }
nit: I've found the nestedness a bit overwhelming, wdyt about putting this into
a scope_exit function and using early returns above ?
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:120
+ I != E; ++I) {
+// Avoid weird non-substring cases like phantom "." components.
+if (I->end() < Parent.begin() && I->end() > Parent.end())
ummm, what ? is this a bug in path iterator that should be fixed rather than
worked around?
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:181
+ for (const auto : getFragments(P, DC))
+Fragment(P, C);
+ return C;
nit: I think this looks a bit ... surprising
Comment at: clang-tools-extra/clangd/ConfigProvider.h:72
+
+ /// A provider that includes fragments from all the supplied providers.
+ static std::unique_ptr
maybe also mention precedence order ? I suppose it already has the natural, the
latter one will override the former behavior, but saying out explicitly
wouldn't hurt.
Comment at: clang-tools-extra/clangd/ConfigProvider.h:74
+ static std::unique_ptr
+ combine(std::vector>);
+
maybe `chain` instead of `combine` ?
Comment at: clang-tools-extra/clangd/ConfigProvider.h:85
+ /// When parsing/compiling, the DiagnosticCallback is used to report errors.
+ /// Usual thread-safety guarantees apply: this function must be threadsafe.
+ virtual std::vector
first part of this sentence doesn't say much, maybe drop it?
Comment at: clang-tools-extra/clangd/ConfigProvider.h:91
+/// A provider that reads config from a YAML file in ~/.config/clangd.
+std::unique_ptr createFileConfigProvider(const ThreadsafeFS &);
+
doesn't seem to be implemented anywhere? I suppose it is just a convenience
wrapper around `Provider::fromYAMLFile`, is it worth it?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82964/new/
https://reviews.llvm.org/D82964
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82964: [clangd] Config: loading and caching config from disk.
njames93 added inline comments. Comment at: clang-tools-extra/clangd/ConfigProvider.h:59 + + // Reads fragments from a single YAML file with an fixed path. + static std::unique_ptr fromYAMLFile(llvm::StringRef AbsPathPath, `a` fixed path Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82964/new/ https://reviews.llvm.org/D82964 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82964: [clangd] Config: loading and caching config from disk.
sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added subscribers: cfe-commits, usaxena95, jfb, arphaman, jkorous,
MaskRay, ilya-biryukov, mgorny.
Herald added a project: clang.
The Provider extension point is designed to also be implemented by
ClangdLSPServer (to inject config-over-lsp) and likely by embedders.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D82964
Files:
clang-tools-extra/clangd/CMakeLists.txt
clang-tools-extra/clangd/ConfigProvider.cpp
clang-tools-extra/clangd/ConfigProvider.h
clang-tools-extra/clangd/unittests/CMakeLists.txt
clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
Index: clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
@@ -0,0 +1,154 @@
+//===-- ConfigProviderTests.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Config.h"
+#include "ConfigProvider.h"
+#include "ConfigTesting.h"
+#include "TestFS.h"
+#include "llvm/Support/SourceMgr.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include
+
+namespace clang {
+namespace clangd {
+namespace config {
+namespace {
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+
+// Provider that appends an arg to compile flags.
+// The arg is prefix, where N is the times getFragments() was called.
+// It also yields a diagnostic each time it's called.
+class FakeProvider : public Provider {
+ std::string Prefix;
+ mutable std::atomic Index = {0};
+
+ std::vector
+ getFragments(const Params &, DiagnosticCallback DC) const override {
+DC(llvm::SMDiagnostic("", llvm::SourceMgr::DK_Error, Prefix));
+CompiledFragment F =
+[Arg(Prefix + std::to_string(++Index))](const Params , Config ) {
+ C.CompileFlags.Edits.push_back(
+ [Arg](std::vector ) { Argv.push_back(Arg); });
+ return true;
+};
+return {F};
+ }
+
+public:
+ FakeProvider(llvm::StringRef Prefix) : Prefix(Prefix) {}
+};
+
+std::vector getAddedArgs(Config ) {
+ std::vector Argv;
+ for (auto : C.CompileFlags.Edits)
+Edit(Argv);
+ return Argv;
+}
+
+// The provider from combine() should invoke its providers in order, and not
+// cache their results.
+TEST(ProviderTest, Combine) {
+ CapturedDiags Diags;
+ std::vector> Providers;
+ Providers.push_back(std::make_unique("foo"));
+ Providers.push_back(std::make_unique("bar"));
+ auto Combined = Provider::combine(std::move(Providers));
+ Config Cfg = Combined->getConfig(Params(), Diags.callback());
+ EXPECT_THAT(Diags.Diagnostics,
+ ElementsAre(DiagMessage("foo"), DiagMessage("bar")));
+ EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo1", "bar1"));
+ Diags.Diagnostics.clear();
+
+ Cfg = Combined->getConfig(Params(), Diags.callback());
+ EXPECT_THAT(Diags.Diagnostics,
+ ElementsAre(DiagMessage("foo"), DiagMessage("bar")));
+ EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo2", "bar2"));
+}
+
+const char *AddFooWithErr = R"yaml(
+CompileFlags:
+ Add: foo
+ Unknown: 42
+)yaml";
+
+const char *AddBarBaz = R"yaml(
+CompileFlags:
+ Add: bar
+---
+CompileFlags:
+ Add: baz
+)yaml";
+
+TEST(ProviderTest, FromYAMLFile) {
+ MockFS FS;
+ FS.Files["foo.yaml"] = AddFooWithErr;
+
+ CapturedDiags Diags;
+ auto P = Provider::fromYAMLFile(testPath("foo.yaml"), FS);
+ auto Cfg = P->getConfig(Params(), Diags.callback());
+ EXPECT_THAT(Diags.Diagnostics,
+ ElementsAre(DiagMessage("Unknown CompileFlags key Unknown")));
+ EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo"));
+ Diags.Diagnostics.clear();
+
+ Cfg = P->getConfig(Params(), Diags.callback());
+ EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "Cached, not re-parsed";
+ EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo"));
+
+ FS.Files["foo.yaml"] = AddBarBaz;
+ Cfg = P->getConfig(Params(), Diags.callback());
+ EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "New config, no errors";
+ EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("bar", "baz"));
+
+ FS.Files.erase("foo.yaml");
+ Cfg = P->getConfig(Params(), Diags.callback());
+ EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "Missing file is not an error";
+ EXPECT_THAT(getAddedArgs(Cfg), IsEmpty());
+}
+
+TEST(ProviderTest, FromAncestorRelativeYAMLFiles) {
+ MockFS FS;
+ FS.Files["a/b/c/foo.yaml"] = AddBarBaz;
+ FS.Files["a/foo.yaml"] = AddFooWithErr;
+
+ std::string ABCPath = testPath("a/b/c/d/test.cc");
+ Params ABCParams;
+ ABCParams.Path = ABCPath;
+ std::string APath = testPath("a/b/e/f/test.cc");
+ Params AParams;
+ AParams.Path = APath;
+
+ CapturedDiags Diags;
+ auto P =
