kadircet updated this revision to Diff 495159.
kadircet marked 2 inline comments as done.
kadircet added a comment.
- Address comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D142890/new/
https://reviews.llvm.org/D142890
Files:
clang-tools-extra/clangd/Config.h
clang-tools-extra/clangd/ConfigCompile.cpp
clang-tools-extra/clangd/ConfigFragment.h
clang-tools-extra/clangd/ConfigYAML.cpp
clang-tools-extra/clangd/Preamble.cpp
clang-tools-extra/clangd/Preamble.h
clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
clang-tools-extra/clangd/unittests/PreambleTests.cpp
Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ clang-tools-extra/clangd/unittests/PreambleTests.cpp
@@ -8,9 +8,13 @@
#include "Annotations.h"
#include "Compiler.h"
+#include "Config.h"
+#include "Diagnostics.h"
#include "Headers.h"
#include "Hover.h"
+#include "ParsedAST.h"
#include "Preamble.h"
+#include "Protocol.h"
#include "SourceCode.h"
#include "TestFS.h"
#include "TestTU.h"
@@ -19,10 +23,12 @@
#include "clang/Format/Format.h"
#include "clang/Frontend/FrontendActions.h"
#include "clang/Frontend/PrecompiledPreamble.h"
+#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/VirtualFileSystem.h"
+#include "llvm/Testing/Support/SupportHelpers.h"
#include "gmock/gmock.h"
#include "gtest/gtest-matchers.h"
#include "gtest/gtest.h"
@@ -32,9 +38,14 @@
#include <vector>
using testing::Contains;
+using testing::ElementsAre;
using testing::Field;
+using testing::IsEmpty;
using testing::Matcher;
using testing::MatchesRegex;
+using testing::Not;
+using testing::UnorderedElementsAre;
+using testing::UnorderedElementsAreArray;
namespace clang {
namespace clangd {
@@ -197,9 +208,12 @@
Field(&Inclusion::FileKind, SrcMgr::CharacteristicKind::C_User))));
}
-std::optional<ParsedAST> createPatchedAST(llvm::StringRef Baseline,
- llvm::StringRef Modified) {
- auto BaselinePreamble = TestTU::withCode(Baseline).preamble();
+std::optional<ParsedAST>
+createPatchedAST(llvm::StringRef Baseline, llvm::StringRef Modified,
+ llvm::StringMap<std::string> AdditionalFiles = {}) {
+ auto PreambleTU = TestTU::withCode(Baseline);
+ PreambleTU.AdditionalFiles = AdditionalFiles;
+ auto BaselinePreamble = PreambleTU.preamble();
if (!BaselinePreamble) {
ADD_FAILURE() << "Failed to build baseline preamble";
return std::nullopt;
@@ -208,6 +222,7 @@
IgnoreDiagnostics Diags;
MockFS FS;
auto TU = TestTU::withCode(Modified);
+ TU.AdditionalFiles = std::move(AdditionalFiles);
auto CI = buildCompilerInvocation(TU.inputs(FS), Diags);
if (!CI) {
ADD_FAILURE() << "Failed to build compiler invocation";
@@ -586,6 +601,153 @@
TU.inputs(FS), *BaselinePreamble);
EXPECT_TRUE(PP.text().empty());
}
+
+std::vector<clangd::Range> mainFileDiagRanges(const ParsedAST &AST) {
+ std::vector<clangd::Range> Result;
+ auto AddRangeIfInMainfile = [&Result](const DiagBase &DB) {
+ if (DB.InsideMainFile)
+ Result.emplace_back(DB.Range);
+ };
+ for (auto &D : *AST.getDiagnostics()) {
+ AddRangeIfInMainfile(D);
+ for (auto &N : D.Notes)
+ AddRangeIfInMainfile(N);
+ }
+ return Result;
+}
+
+TEST(PreamblePatch, DiagnosticsFromMainASTAreInRightPlace) {
+ Config Cfg;
+ Cfg.Diagnostics.AllowStalePreamble = true;
+ WithContextValue WithCfg(Config::Key, std::move(Cfg));
+
+ llvm::StringLiteral BaselinePreamble = "#define FOO\n";
+ {
+ // Check with removals from preamble.
+ Annotations Code("[[x]];/* error-ok */");
+ auto AST = createPatchedAST(BaselinePreamble, Code.code());
+ EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(Code.range()));
+ }
+ {
+ // Check with additions to preamble.
+ Annotations Code((BaselinePreamble + R"(
+ #define BAR
+ [[x]];/* error-ok */)")
+ .str());
+ auto AST = createPatchedAST(BaselinePreamble, Code.code());
+ EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(Code.range()));
+ }
+}
+
+TEST(PreamblePatch, DiagnosticsToPreamble) {
+ Config Cfg;
+ Cfg.Diagnostics.AllowStalePreamble = true;
+ Cfg.Diagnostics.UnusedIncludes = Config::UnusedIncludesPolicy::Strict;
+ WithContextValue WithCfg(Config::Key, std::move(Cfg));
+
+ llvm::StringMap<std::string> AdditionalFiles;
+ AdditionalFiles["foo.h"] = "#pragma once";
+ AdditionalFiles["bar.h"] = "#pragma once";
+ llvm::StringLiteral BaselinePreamble(R"(
+ // Test comment
+ [[#include "foo.h"]])");
+ {
+ // Check with removals from preamble.
+ Annotations NewCode("[[# include \"foo.h\"]]");
+ auto AST = createPatchedAST(Annotations(BaselinePreamble).code(),
+ NewCode.code(), AdditionalFiles);
+ EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(NewCode.range()));
+ }
+ {
+ // Check with additions to preamble.
+ Annotations Code(BaselinePreamble);
+ Annotations NewCode(("[[#include \"bar.h\"]]\n" + BaselinePreamble).str());
+ auto AST = createPatchedAST(Code.code(), NewCode.code(), AdditionalFiles);
+ EXPECT_THAT(mainFileDiagRanges(*AST),
+ UnorderedElementsAreArray(NewCode.ranges()));
+ }
+ {
+ llvm::StringLiteral BaselinePreamble = "#define [[FOO]] 1\n";
+ Annotations Code(BaselinePreamble);
+ // Check ranges for notes.
+ Annotations NewCode(("#define [[BARXYZ]] 1\n" + BaselinePreamble +
+ "void foo();\n#define [[FOO]] 2")
+ .str());
+ auto AST = createPatchedAST(Code.code(), NewCode.code(), AdditionalFiles);
+ // FIXME: We shouldn't be warning for BARXYZ, and pointing at the FOO inside
+ // the baselinepreamble.
+ EXPECT_THAT(mainFileDiagRanges(*AST),
+ UnorderedElementsAre(NewCode.ranges()[0], NewCode.ranges()[2]));
+ }
+}
+
+TEST(PreamblePatch, TranslatesIncludeDiagnosticsInPreamble) {
+ Config Cfg;
+ Cfg.Diagnostics.AllowStalePreamble = true;
+ WithContextValue WithCfg(Config::Key, std::move(Cfg));
+
+ llvm::StringLiteral BaselinePreamble("#include [[<foo>]]\n");
+ {
+ // Check with additions to preamble.
+ Annotations Code(BaselinePreamble);
+ Annotations NewCode(("#define BAR\n" + BaselinePreamble).str());
+ auto AST = createPatchedAST(Code.code(), NewCode.code());
+ // FIXME: We should point at the NewCode.
+ EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(Code.range()));
+ }
+ {
+ // Check with removals from preamble.
+ Annotations Code(("#define BAR\n" + BaselinePreamble).str());
+ Annotations NewCode(BaselinePreamble);
+ auto AST = createPatchedAST(Code.code(), NewCode.code());
+ // FIXME: We should point at the NewCode.
+ EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(Code.range()));
+ }
+ {
+ // Drop line with diags.
+ Annotations Code(BaselinePreamble);
+ Annotations NewCode("#define BAR\n#include [[<bar>]]\n");
+ auto AST = createPatchedAST(Code.code(), NewCode.code());
+ // FIXME: We should point at the NewCode.
+ EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(Code.range()));
+ }
+}
+
+TEST(PreamblePatch, TranslatesNonIncludeDiagnosticsInPreamble) {
+ Config Cfg;
+ Cfg.Diagnostics.AllowStalePreamble = true;
+ WithContextValue WithCfg(Config::Key, std::move(Cfg));
+
+ llvm::StringLiteral BaselinePreamble("#define [[__LINE__]]\n");
+ ASSERT_THAT(TestTU::withCode(Annotations(BaselinePreamble).code())
+ .build()
+ .getDiagnostics(),
+ Not(llvm::ValueIs(IsEmpty())));
+ {
+ // Check with additions to preamble.
+ Annotations Code(BaselinePreamble);
+ Annotations NewCode(("#define BAR\n" + BaselinePreamble).str());
+ auto AST = createPatchedAST(Code.code(), NewCode.code());
+ // FIXME: We should point at the NewCode.
+ EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(Code.range()));
+ }
+ {
+ // Check with removals from preamble.
+ Annotations Code(("#define BAR\n" + BaselinePreamble).str());
+ Annotations NewCode(BaselinePreamble);
+ auto AST = createPatchedAST(Code.code(), NewCode.code());
+ // FIXME: We should point at the NewCode.
+ EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(Code.range()));
+ }
+ {
+ // Drop line with diags.
+ Annotations Code(BaselinePreamble);
+ Annotations NewCode("#define BAR\n#include [[<bar>]]\n#include <baz>\n");
+ auto AST = createPatchedAST(Code.code(), NewCode.code());
+ // FIXME: We should point at the NewCode.
+ EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(Code.range()));
+ }
+}
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -538,7 +538,7 @@
void foo() {}
)cpp");
Config Cfg;
- Cfg.Diagnostics.UnusedIncludes = Config::Experiment;
+ Cfg.Diagnostics.UnusedIncludes = Config::UnusedIncludesPolicy::Experiment;
WithContextValue Ctx(Config::Key, std::move(Cfg));
ParsedAST AST = TU.build();
@@ -627,7 +627,7 @@
TU.ExtraArgs.emplace_back("-xobjective-c");
Config Cfg;
- Cfg.Diagnostics.UnusedIncludes = Config::Strict;
+ Cfg.Diagnostics.UnusedIncludes = Config::UnusedIncludesPolicy::Strict;
WithContextValue Ctx(Config::Key, std::move(Cfg));
ParsedAST AST = TU.build();
EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -273,6 +273,33 @@
EXPECT_THAT(Results[0].Style.FullyQualifiedNamespaces,
ElementsAre(val("foo"), val("bar")));
}
+
+TEST(ParseYAML, DiagnosticsMode) {
+ CapturedDiags Diags;
+ {
+ Annotations YAML(R"yaml(
+Diagnostics:
+ AllowStalePreamble: Yes)yaml");
+ auto Results =
+ Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
+ ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+ ASSERT_EQ(Results.size(), 1u);
+ EXPECT_THAT(Results[0].Diagnostics.AllowStalePreamble,
+ llvm::ValueIs(val(true)));
+ }
+
+ {
+ Annotations YAML(R"yaml(
+Diagnostics:
+ AllowStalePreamble: No)yaml");
+ auto Results =
+ Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
+ ASSERT_THAT(Diags.Diagnostics, IsEmpty());
+ ASSERT_EQ(Results.size(), 1u);
+ EXPECT_THAT(Results[0].Diagnostics.AllowStalePreamble,
+ llvm::ValueIs(val(false)));
+ }
+}
} // namespace
} // namespace config
} // namespace clangd
Index: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
@@ -543,6 +543,21 @@
EXPECT_TRUE(compileAndApply());
EXPECT_THAT(Conf.Style.FullyQualifiedNamespaces, ElementsAre("foo", "bar"));
}
+
+TEST_F(ConfigCompileTests, AllowDiagsFromStalePreamble) {
+ Frag = {};
+ EXPECT_TRUE(compileAndApply());
+ // Off by default.
+ EXPECT_EQ(Conf.Diagnostics.AllowStalePreamble, false);
+
+ Frag.Diagnostics.AllowStalePreamble.emplace(true);
+ EXPECT_TRUE(compileAndApply());
+ EXPECT_EQ(Conf.Diagnostics.AllowStalePreamble, true);
+
+ Frag.Diagnostics.AllowStalePreamble.emplace(false);
+ EXPECT_TRUE(compileAndApply());
+ EXPECT_EQ(Conf.Diagnostics.AllowStalePreamble, false);
+}
} // namespace
} // namespace config
} // namespace clangd
Index: clang-tools-extra/clangd/Preamble.h
===================================================================
--- clang-tools-extra/clangd/Preamble.h
+++ clang-tools-extra/clangd/Preamble.h
@@ -155,7 +155,7 @@
llvm::StringRef text() const { return PatchContents; }
/// Whether diagnostics generated using this patch are trustable.
- bool preserveDiagnostics() const { return PatchContents.empty(); }
+ bool preserveDiagnostics() const;
private:
static PreamblePatch create(llvm::StringRef FileName,
Index: clang-tools-extra/clangd/Preamble.cpp
===================================================================
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -790,5 +790,9 @@
return Loc;
}
+bool PreamblePatch::preserveDiagnostics() const {
+ return PatchContents.empty() ||
+ Config::current().Diagnostics.AllowStalePreamble;
+}
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/ConfigYAML.cpp
===================================================================
--- clang-tools-extra/clangd/ConfigYAML.cpp
+++ clang-tools-extra/clangd/ConfigYAML.cpp
@@ -130,6 +130,9 @@
});
Dict.handle("Includes", [&](Node &N) { parse(F.Includes, N); });
Dict.handle("ClangTidy", [&](Node &N) { parse(F.ClangTidy, N); });
+ Dict.handle("AllowStalePreamble", [&](Node &N) {
+ F.AllowStalePreamble = boolValue(N, "AllowStalePreamble");
+ });
Dict.parse(N);
}
@@ -268,7 +271,7 @@
// If Key is seen twice, Parse runs only once and an error is reported.
void handle(llvm::StringLiteral Key, std::function<void(Node &)> Parse) {
for (const auto &Entry : Keys) {
- (void) Entry;
+ (void)Entry;
assert(Entry.first != Key && "duplicate key handler");
}
Keys.emplace_back(Key, std::move(Parse));
Index: clang-tools-extra/clangd/ConfigFragment.h
===================================================================
--- clang-tools-extra/clangd/ConfigFragment.h
+++ clang-tools-extra/clangd/ConfigFragment.h
@@ -232,9 +232,13 @@
///
/// Valid values are:
/// - Strict
+ /// - Experiment
/// - None
std::optional<Located<std::string>> UnusedIncludes;
+ /// Enable emitting diagnostics using stale preambles.
+ std::optional<Located<bool>> AllowStalePreamble;
+
/// Controls IncludeCleaner diagnostics.
struct IncludesBlock {
/// Regexes that will be used to avoid diagnosing certain includes as
Index: clang-tools-extra/clangd/ConfigCompile.cpp
===================================================================
--- clang-tools-extra/clangd/ConfigCompile.cpp
+++ clang-tools-extra/clangd/ConfigCompile.cpp
@@ -441,8 +441,14 @@
Out.Apply.push_back([Val](const Params &, Config &C) {
C.Diagnostics.UnusedIncludes = *Val;
});
- compile(std::move(F.Includes));
+ if (F.AllowStalePreamble) {
+ if (auto Val = F.AllowStalePreamble)
+ Out.Apply.push_back([Val](const Params &, Config &C) {
+ C.Diagnostics.AllowStalePreamble = **Val;
+ });
+ }
+ compile(std::move(F.Includes));
compile(std::move(F.ClangTidy));
}
Index: clang-tools-extra/clangd/Config.h
===================================================================
--- clang-tools-extra/clangd/Config.h
+++ clang-tools-extra/clangd/Config.h
@@ -88,7 +88,7 @@
bool StandardLibrary = true;
} Index;
- enum UnusedIncludesPolicy {
+ enum class UnusedIncludesPolicy {
/// Diagnose unused includes.
Strict,
None,
@@ -107,7 +107,10 @@
llvm::StringMap<std::string> CheckOptions;
} ClangTidy;
- UnusedIncludesPolicy UnusedIncludes = None;
+ UnusedIncludesPolicy UnusedIncludes = UnusedIncludesPolicy::None;
+
+ /// Enable emitting diagnostics using stale preambles.
+ bool AllowStalePreamble = false;
/// IncludeCleaner will not diagnose usages of these headers matched by
/// these regexes.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
