kadircet updated this revision to Diff 195375.
kadircet marked 14 inline comments as done.
kadircet added a comment.
- Address comments
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59302/new/
https://reviews.llvm.org/D59302
Files:
clangd/Diagnostics.cpp
clangd/Diagnostics.h
unittests/clangd/DiagnosticsTests.cpp
unittests/clangd/TestTU.cpp
unittests/clangd/TestTU.h
Index: unittests/clangd/TestTU.h
===================================================================
--- unittests/clangd/TestTU.h
+++ unittests/clangd/TestTU.h
@@ -18,8 +18,11 @@
#define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTTU_H
#include "ClangdUnit.h"
+#include "Path.h"
#include "index/Index.h"
#include "gtest/gtest.h"
+#include <utility>
+#include <vector>
namespace clang {
namespace clangd {
@@ -45,6 +48,9 @@
std::string HeaderCode;
std::string HeaderFilename = "TestTU.h";
+ // Absolute path and contents of each file.
+ std::vector<std::pair<Path, std::string>> AdditionalFiles;
+
// Extra arguments for the compiler invocation.
std::vector<const char *> ExtraArgs;
Index: unittests/clangd/TestTU.cpp
===================================================================
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -20,6 +20,13 @@
ParsedAST TestTU::build() const {
std::string FullFilename = testPath(Filename),
FullHeaderName = testPath(HeaderFilename);
+
+ llvm::StringMap<std::string> Files;
+ Files[FullFilename] = Code;
+ Files[FullHeaderName] = HeaderCode;
+ for (const auto &Entry : AdditionalFiles)
+ Files[Entry.first] = Entry.second;
+
std::vector<const char *> Cmd = {"clang", FullFilename.c_str()};
// FIXME: this shouldn't need to be conditional, but it breaks a
// GoToDefinition test for some reason (getMacroArgExpandedLocation fails).
@@ -33,7 +40,7 @@
Inputs.CompileCommand.CommandLine = {Cmd.begin(), Cmd.end()};
Inputs.CompileCommand.Directory = testRoot();
Inputs.Contents = Code;
- Inputs.FS = buildTestFS({{FullFilename, Code}, {FullHeaderName, HeaderCode}});
+ Inputs.FS = buildTestFS(Files);
Inputs.Opts = ParseOptions();
Inputs.Opts.ClangTidyOpts.Checks = ClangTidyChecks;
Inputs.Index = ExternalIndex;
Index: unittests/clangd/DiagnosticsTests.cpp
===================================================================
--- unittests/clangd/DiagnosticsTests.cpp
+++ unittests/clangd/DiagnosticsTests.cpp
@@ -8,13 +8,17 @@
#include "Annotations.h"
#include "ClangdUnit.h"
+#include "Path.h"
+#include "Protocol.h"
#include "SourceCode.h"
+#include "TestFS.h"
#include "TestIndex.h"
#include "TestTU.h"
#include "index/MemIndex.h"
#include "llvm/Support/ScopedPrinter.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
+#include <algorithm>
namespace clang {
namespace clangd {
@@ -45,6 +49,15 @@
return arg.Range == Range && arg.Message == Message;
}
+MATCHER_P3(Diag, Range, Message, IncludeStack,
+ "Diag at " + llvm::to_string(Range) + " = [" + Message + "]") {
+ if (arg.Range != Range || arg.Message != Message ||
+ arg.IncludeStack.size() != IncludeStack.size())
+ return false;
+ return std::equal(IncludeStack.begin(), IncludeStack.end(),
+ arg.IncludeStack.begin());
+}
+
MATCHER_P3(Fix, Range, Replacement, Message,
"Fix " + llvm::to_string(Range) + " => " +
testing::PrintToString(Replacement) + " = [" + Message + "]") {
@@ -73,7 +86,6 @@
return true;
}
-
// Helper function to make tests shorter.
Position pos(int line, int character) {
Position Res;
@@ -251,6 +263,8 @@
D.InsideMainFile = true;
D.Severity = DiagnosticsEngine::Error;
D.File = "foo/bar/main.cpp";
+ D.IncludeStack.push_back({"a/b.h", pos(0, 1)});
+ D.IncludeStack.push_back({"a/c.h", pos(1, 1)});
clangd::Note NoteInMain;
NoteInMain.Message = "declared somewhere in the main file";
@@ -281,8 +295,9 @@
};
// Diagnostics should turn into these:
- clangd::Diagnostic MainLSP =
- MatchingLSP(D, R"(Something terrible happened (fix available)
+ clangd::Diagnostic MainLSP = MatchingLSP(
+ D, R"(In file a/c.h:2:2: something terrible happened (fix available)
+In file included from: a/b.h:1:2
main.cpp:6:7: remark: declared somewhere in the main file
@@ -603,7 +618,144 @@
"Add include \"x.h\" for symbol a::X")))));
}
+TEST(DiagsInHeaders, DiagInsideHeader) {
+ Annotations Main(R"cpp(
+ #include [["a.h"]]
+ void foo() {})cpp");
+ TestTU TU = TestTU::withCode(Main.code());
+ TU.AdditionalFiles = {{testPath("a.h"), "no_type_spec;"}};
+ EXPECT_THAT(TU.build().getDiagnostics(),
+ UnorderedElementsAre(
+ Diag(Main.range(),
+ "C++ requires a type specifier for all declarations",
+ std::vector<std::pair<Path, Position>>{
+ {"/clangd-test/a.h", pos(0, 0)}})));
+}
+
+TEST(DiagsInHeaders, DiagInTransitiveInclude) {
+ Annotations Main(R"cpp(
+ #include [["a.h"]]
+ void foo() {})cpp");
+ TestTU TU = TestTU::withCode(Main.code());
+ TU.AdditionalFiles = {{testPath("a.h"), "#include \"b.h\""},
+ {testPath("b.h"), "no_type_spec;"}};
+ EXPECT_THAT(TU.build().getDiagnostics(),
+ UnorderedElementsAre(
+ Diag(Main.range(),
+ "C++ requires a type specifier for all declarations",
+ std::vector<std::pair<Path, Position>>{
+ {"/clangd-test/a.h", pos(0, 9)},
+ {"/clangd-test/b.h", pos(0, 0)}})));
+}
+
+TEST(DiagsInHeaders, DiagInMultipleHeaders) {
+ Annotations Main(R"cpp(
+ #include $a[["a.h"]]
+ #include $b[["b.h"]]
+ void foo() {})cpp");
+ TestTU TU = TestTU::withCode(Main.code());
+ TU.AdditionalFiles = {{testPath("a.h"), "no_type_spec;"},
+ {testPath("b.h"), "no_type_spec;"}};
+ EXPECT_THAT(TU.build().getDiagnostics(),
+ UnorderedElementsAre(
+ Diag(Main.range("a"),
+ "C++ requires a type specifier for all declarations",
+ std::vector<std::pair<Path, Position>>{
+ {"/clangd-test/a.h", pos(0, 0)}}),
+ Diag(Main.range("b"),
+ "C++ requires a type specifier for all declarations",
+ std::vector<std::pair<Path, Position>>{
+ {"/clangd-test/b.h", pos(0, 0)}})));
+}
+
+TEST(DiagsInHeaders, PreferExpansionLocation) {
+ Annotations Main(R"cpp(
+ #include [["a.h"]]
+ #include "b.h"
+ void foo() {})cpp");
+ TestTU TU = TestTU::withCode(Main.code());
+ TU.AdditionalFiles = {
+ {testPath("a.h"), "#include \"b.h\"\n"},
+ {testPath("b.h"), "#ifndef X\n#define X\nno_type_spec;\n#endif"}};
+ EXPECT_THAT(TU.build().getDiagnostics(),
+ UnorderedElementsAre(
+ Diag(Main.range(),
+ "C++ requires a type specifier for all declarations",
+ std::vector<std::pair<Path, Position>>{
+ {"/clangd-test/a.h", pos(0, 9)},
+ {"/clangd-test/b.h", pos(2, 0)}})));
+}
+
+TEST(DiagsInHeaders, PreferExpansionLocationMacros) {
+ Annotations Main(R"cpp(
+ #define X
+ #include "a.h"
+ #undef X
+ #include [["b.h"]]
+ void foo() {})cpp");
+ TestTU TU = TestTU::withCode(Main.code());
+ TU.AdditionalFiles = {
+ {testPath("a.h"), "#include \"c.h\"\n"},
+ {testPath("b.h"), "#include \"c.h\"\n"},
+ {testPath("c.h"), "#ifndef X\n#define X\nno_type_spec;\n#endif"}};
+ EXPECT_THAT(TU.build().getDiagnostics(),
+ UnorderedElementsAre(
+ Diag(Main.range(),
+ "C++ requires a type specifier for all declarations",
+ std::vector<std::pair<Path, Position>>{
+ {"/clangd-test/b.h", pos(0, 9)},
+ {"/clangd-test/c.h", pos(2, 0)}})));
+}
+
+TEST(DiagsInHeaders, LimitDiagsOutsideMainFile) {
+ Annotations Main(R"cpp(
+ #include [["a.h"]]
+ #include "b.h"
+ void foo() {})cpp");
+ TestTU TU = TestTU::withCode(Main.code());
+ TU.AdditionalFiles = {{testPath("a.h"), "#include \"c.h\"\n"},
+ {testPath("b.h"), "#include \"c.h\"\n"},
+ {testPath("c.h"), R"cpp(
+ #ifndef X
+ #define X
+ no_type_spec_0;
+ no_type_spec_1;
+ no_type_spec_2;
+ no_type_spec_3;
+ no_type_spec_4;
+ no_type_spec_5;
+ no_type_spec_6;
+ no_type_spec_7;
+ no_type_spec_8;
+ no_type_spec_9;
+ no_type_spec_10;
+ #endif)cpp"}};
+ EXPECT_THAT(TU.build().getDiagnostics(),
+ UnorderedElementsAre(
+ Diag(Main.range(),
+ "C++ requires a type specifier for all declarations",
+ std::vector<std::pair<Path, Position>>{
+ {"/clangd-test/a.h", pos(0, 9)},
+ {"/clangd-test/c.h", pos(3, 6)}})));
+}
+
+TEST(DiagsInHeaders, OnlyErrorOrFatal) {
+ Annotations Main(R"cpp(
+ #include [["a.h"]]
+ void foo() {})cpp");
+ TestTU TU = TestTU::withCode(Main.code());
+ TU.AdditionalFiles = {{testPath("a.h"),
+ R"cpp(no_type_spec;
+ int x = 5/0;)cpp"}};
+ auto diags = TU.build().getDiagnostics();
+ EXPECT_THAT(TU.build().getDiagnostics(),
+ UnorderedElementsAre(
+ Diag(Main.range(),
+ "C++ requires a type specifier for all declarations",
+ std::vector<std::pair<Path, Position>>{
+ {"/clangd-test/a.h", pos(0, 0)}})));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
-
Index: clangd/Diagnostics.h
===================================================================
--- clangd/Diagnostics.h
+++ clangd/Diagnostics.h
@@ -14,6 +14,9 @@
#include "clang/Basic/Diagnostic.h"
#include "clang/Basic/LangOptions.h"
#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/StringSet.h"
#include <cassert>
@@ -80,6 +83,8 @@
std::vector<Note> Notes;
/// *Alternative* fixes for this diagnostic, one should be chosen.
std::vector<Fix> Fixes;
+ /// The include closest to main file is in front.
+ std::vector<std::pair<Path, Position>> IncludeStack;
};
llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Diag &D);
@@ -123,6 +128,8 @@
std::vector<Diag> Output;
llvm::Optional<LangOptions> LangOpts;
llvm::Optional<Diag> LastDiag;
+ /// Stores lines of the includes containing a diagnostic.
+ llvm::DenseSet<int> IncludeHasDiag;
};
} // namespace clangd
Index: clangd/Diagnostics.cpp
===================================================================
--- clangd/Diagnostics.cpp
+++ clangd/Diagnostics.cpp
@@ -10,10 +10,16 @@
#include "Compiler.h"
#include "Logger.h"
#include "SourceCode.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/SourceManager.h"
#include "clang/Lex/Lexer.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Capacity.h"
#include "llvm/Support/Path.h"
+#include "llvm/Support/Signals.h"
#include <algorithm>
namespace clang {
@@ -34,6 +40,47 @@
return false;
}
+void adjustDiagFromHeader(Diag &D, const clang::Diagnostic &Info,
+ const LangOptions &LangOpts) {
+ const SourceLocation &DiagLoc = Info.getLocation();
+ const SourceManager &SM = Info.getSourceManager();
+ std::vector<SourceLocation> IncludeStack;
+ auto GetIncludeLoc = [&SM](SourceLocation SLoc) {
+ return SM.getIncludeLoc(SM.getFileID(SLoc));
+ };
+ for (auto IncludeLocation = GetIncludeLoc(DiagLoc); IncludeLocation.isValid();
+ IncludeLocation = GetIncludeLoc(IncludeLocation)) {
+ IncludeStack.push_back(IncludeLocation);
+ }
+ if (IncludeStack.empty())
+ return;
+ D.File = SM.getFileEntryForID(SM.getMainFileID())->getName().str();
+ D.Range.start = sourceLocToPosition(SM, IncludeStack.back());
+ D.Range.end = sourceLocToPosition(
+ SM, Lexer::getLocForEndOfToken(IncludeStack.back(), 0, SM, LangOpts));
+ // We don't show the main file as part of include stack, which is the
+ // last element.
+ if (IncludeStack.size() > 1) {
+ IncludeStack.pop_back();
+ std::reverse(IncludeStack.begin(), IncludeStack.end());
+ for (auto &Inc : IncludeStack)
+ D.IncludeStack.push_back(
+ {SM.getFileEntryForID(SM.getFileID(Inc))->getName().str(),
+ sourceLocToPosition(SM, Inc)});
+ }
+ D.IncludeStack.push_back(
+ {SM.getFileEntryForID(SM.getFileID(DiagLoc))->getName().str(),
+ sourceLocToPosition(SM, DiagLoc)});
+}
+
+// Position stores line and character offset 0-based. But we want 1-based
+// offsets when displaying that information to users.
+Position toOneBased(Position P) {
+ ++P.line;
+ ++P.character;
+ return P;
+}
+
// Checks whether a location is within a half-open range.
// Note that clang also uses closed source ranges, which this can't handle!
bool locationInRange(SourceLocation L, CharSourceRange R,
@@ -131,8 +178,7 @@
}
// Note +1 to line and character. clangd::Range is zero-based, but when
// printing for users we want one-based indexes.
- auto Pos = D.Range.start;
- OS << (Pos.line + 1) << ":" << (Pos.character + 1) << ":";
+ OS << toOneBased(D.Range.start) << ":";
// The non-main-file paths are often too long, putting them on a separate
// line improves readability.
if (D.InsideMainFile)
@@ -164,9 +210,21 @@
std::string mainMessage(const Diag &D, bool DisplayFixesCount) {
std::string Result;
llvm::raw_string_ostream OS(Result);
+ // Prepend filename if diag is coming from a header.
+ if (!D.IncludeStack.empty()) {
+ const auto &Inc = D.IncludeStack.back();
+ OS << "In file " << Inc.first << ':' << toOneBased(Inc.second) << ": ";
+ }
OS << D.Message;
if (DisplayFixesCount && !D.Fixes.empty())
OS << " (" << (D.Fixes.size() > 1 ? "fixes" : "fix") << " available)";
+ // We don't want to print last file in the include stack, since it has already
+ // been prepended to D.Message.
+ for (size_t I = 0, E = D.IncludeStack.size(); I + 1 < E; ++I) {
+ const auto &Inc = D.IncludeStack[I];
+ OS << "\nIn file included from: " << Inc.first << ':'
+ << toOneBased(Inc.second);
+ }
for (auto &Note : D.Notes) {
OS << "\n\n";
printDiag(OS, Note);
@@ -379,6 +437,7 @@
LastDiag = Diag();
LastDiag->ID = Info.getID();
FillDiagBase(*LastDiag);
+ adjustDiagFromHeader(*LastDiag, Info, *LangOpts);
if (!Info.getFixItHints().empty())
AddFix(true /* try to invent a message instead of repeating the diag */);
@@ -413,11 +472,19 @@
void StoreDiags::flushLastDiag() {
if (!LastDiag)
return;
- if (mentionsMainFile(*LastDiag))
+ auto IsImportantHeaderDiag = [](const Diag &D) {
+ return D.Severity == DiagnosticsEngine::Level::Error ||
+ D.Severity == DiagnosticsEngine::Level::Fatal;
+ };
+ // Only keeps diagnostics inside main file or the first one coming from a
+ // header.
+ if (mentionsMainFile(*LastDiag) ||
+ (IsImportantHeaderDiag(*LastDiag) &&
+ IncludeHasDiag.insert(LastDiag->Range.start.line).second)) {
Output.push_back(std::move(*LastDiag));
- else
- vlog("Dropped diagnostic outside main file: {0}: {1}", LastDiag->File,
- LastDiag->Message);
+ } else {
+ vlog("Dropped diagnostic: {0}: {1}", LastDiag->File, LastDiag->Message);
+ }
LastDiag.reset();
}
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
