kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added a subscriber: arphaman.
Herald added a project: All.
kadircet requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.
This patch tries to just preserve diagnostics that are contained to
lines containing #include or #define directives and moves them around
with the new file contents. It's granularity is per-line, it doesn't
handle column-wide changes.
Depends on D143095 <https://reviews.llvm.org/D143095>
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D143096
Files:
clang-tools-extra/clangd/ParsedAST.cpp
clang-tools-extra/clangd/Preamble.cpp
clang-tools-extra/clangd/Preamble.h
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
@@ -637,17 +637,16 @@
Annotations NewCode("[[# include \"foo.h\"]]");
auto AST = createPatchedAST(Annotations(BaselinePreamble).code(),
NewCode.code(), AdditionalFiles);
- // FIXME: Should be pointing at the range inside the Newcode.
- EXPECT_THAT(mainFileDiagRanges(*AST), IsEmpty());
+ EXPECT_THAT(mainFileDiagRanges(*AST),
+ UnorderedElementsAreArray(NewCode.ranges()));
}
{
// Check with additions to preamble.
Annotations Code(BaselinePreamble);
Annotations NewCode(("[[#include \"barx.h\"]]\n" + BaselinePreamble).str());
auto AST = createPatchedAST(Code.code(), NewCode.code(), AdditionalFiles);
- // FIXME: Should preserve existing diagnostics.
EXPECT_THAT(mainFileDiagRanges(*AST),
- UnorderedElementsAre(NewCode.ranges()[0]));
+ UnorderedElementsAreArray(NewCode.ranges()));
}
{
llvm::StringLiteral BaselinePreamble = "#define [[FOO]] 1\n";
@@ -673,25 +672,21 @@
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()));
+ EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAreArray(NewCode.ranges()));
}
{
// 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()));
+ EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAreArray(NewCode.ranges()));
}
{
// 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 only have ranges from the NewCode.
- EXPECT_THAT(mainFileDiagRanges(*AST),
- ElementsAreArray({Code.range(), NewCode.range()}));
+ EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAreArray(NewCode.ranges()));
}
}
@@ -710,25 +705,21 @@
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()));
+ EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAreArray(NewCode.ranges()));
}
{
// 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()));
+ EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAreArray(NewCode.ranges()));
}
{
// 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 only have the range from NewCode.
- EXPECT_THAT(mainFileDiagRanges(*AST),
- ElementsAreArray({Code.range(), NewCode.range()}));
+ EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAreArray(NewCode.ranges()));
}
}
} // namespace
Index: clang-tools-extra/clangd/Preamble.h
===================================================================
--- clang-tools-extra/clangd/Preamble.h
+++ clang-tools-extra/clangd/Preamble.h
@@ -157,6 +157,9 @@
/// Whether diagnostics generated using this patch are trustable.
bool preserveDiagnostics() const;
+ /// Returns diag locations for Modified contents, only contains diags attached
+ /// to an #include or #define directive.
+ std::vector<Diag> patchedDiags() const { return PatchedDiags; }
static constexpr llvm::StringLiteral HeaderName = "__preamble_patch__.h";
private:
@@ -168,9 +171,11 @@
PreamblePatch() = default;
std::string PatchContents;
std::string PatchFileName;
- /// Includes that are present in both \p Baseline and \p Modified. Used for
- /// patching includes of baseline preamble.
+ // Includes that are present in both \p Baseline and \p Modified. Used for
+ // patching includes of baseline preamble.
std::vector<Inclusion> PreambleIncludes;
+ // Diags that were attached to an #include or a #define directive.
+ std::vector<Diag> PatchedDiags;
PreambleBounds ModifiedBounds = {0, false};
};
Index: clang-tools-extra/clangd/Preamble.cpp
===================================================================
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -9,6 +9,7 @@
#include "Preamble.h"
#include "Compiler.h"
#include "Config.h"
+#include "Diagnostics.h"
#include "Headers.h"
#include "SourceCode.h"
#include "clang-include-cleaner/Record.h"
@@ -35,6 +36,8 @@
#include "llvm/ADT/IntrusiveRefCntPtr.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/ErrorHandling.h"
@@ -43,7 +46,7 @@
#include "llvm/Support/Path.h"
#include "llvm/Support/VirtualFileSystem.h"
#include "llvm/Support/raw_ostream.h"
-#include <iterator>
+#include <map>
#include <memory>
#include <string>
#include <system_error>
@@ -465,6 +468,19 @@
WallTimer Timer;
};
+// Represents an include as spelled in the source code, without taking resolved
+// path or location into account.
+struct IncludeKey {
+ tok::PPKeywordKind Directive;
+ llvm::StringRef Written;
+
+ bool operator<(const IncludeKey &RHS) const {
+ return std::tie(Directive, Written) < std::tie(RHS.Directive, RHS.Written);
+ }
+ bool operator==(const IncludeKey &RHS) const {
+ return std::tie(Directive, Written) == std::tie(RHS.Directive, RHS.Written);
+ }
+};
} // namespace
std::shared_ptr<const PreambleData>
@@ -612,6 +628,70 @@
}
}
+// Checks if all pointers in \p D are for the same line of the main file.
+static bool diagReferencesMultipleLines(const Diag &D) {
+ int Line = D.Range.start.line;
+ if (D.Range.end.line != Line)
+ return true;
+ bool NotePointsToOutside = llvm::any_of(D.Notes, [&](const Note &N) {
+ return N.File == D.File &&
+ (N.Range.start.line != Line || N.Range.end.line != Line);
+ });
+ if (NotePointsToOutside)
+ return true;
+ bool FixPointsToOutside = llvm::any_of(D.Fixes, [&](const Fix &F) {
+ for (auto &E : F.Edits)
+ if (E.range.start.line != Line || E.range.end.line != Line)
+ return true;
+ return false;
+ });
+ if (FixPointsToOutside)
+ return true;
+ return false;
+}
+
+// Move all references in \p D to \p NewLine. This assumes none of the
+// references to the main file are multi-line.
+static Diag translateDiag(const Diag &D, int NewLine) {
+ assert(!diagReferencesMultipleLines(D));
+ Diag Translated = D;
+ Translated.Range.start.line = Translated.Range.end.line = NewLine;
+ for (auto &N : Translated.Notes) {
+ if (N.File != D.File)
+ continue;
+ N.Range.start.line = N.Range.end.line = NewLine;
+ }
+ for (auto &F : Translated.Fixes) {
+ for (auto &E : F.Edits)
+ E.range.start.line = E.range.end.line = NewLine;
+ }
+ return Translated;
+}
+
+static llvm::DenseMap<int, llvm::SmallVector<const Diag *>>
+mapDiagsToLines(llvm::ArrayRef<Diag> Diags) {
+ llvm::DenseMap<int, llvm::SmallVector<const Diag *>> LineToDiags;
+ for (auto &D : Diags) {
+ if (diagReferencesMultipleLines(D))
+ continue;
+ LineToDiags[D.Range.start.line].emplace_back(&D);
+ }
+ return LineToDiags;
+}
+
+static std::map<IncludeKey, const Inclusion *>
+getExistingIncludes(const ScannedPreamble &BaselineScan,
+ llvm::ArrayRef<Inclusion> MFI) {
+ std::map<IncludeKey, const Inclusion *> ExistingIncludes;
+ // There might be includes coming from disabled regions, record these for
+ // exclusion. note that we don't have resolved paths for those.
+ for (const auto &Inc : BaselineScan.Includes)
+ ExistingIncludes[{Inc.Directive, Inc.Written}] = nullptr;
+ for (const auto &Inc : MFI)
+ ExistingIncludes[{Inc.Directive, Inc.Written}] = &Inc;
+ return ExistingIncludes;
+}
+
PreamblePatch PreamblePatch::create(llvm::StringRef FileName,
const ParseInputs &Modified,
const PreambleData &Baseline,
@@ -665,26 +745,37 @@
escapeBackslashAndQuotes(FileName, Patch);
Patch << "\"\n";
+ // List of diagnostics associated to a particular line in the baseline.
+ auto LineToDiags = mapDiagsToLines(Baseline.Diags);
+ auto MoveDiagsBetweenLines = [&LineToDiags, &PP](int OldLine, int NewLine) {
+ auto DiagsForInclude = LineToDiags.find(OldLine);
+ if (DiagsForInclude == LineToDiags.end())
+ return;
+ for (auto *D : DiagsForInclude->second)
+ PP.PatchedDiags.emplace_back(translateDiag(*D, NewLine));
+ };
+ // Map from an include to its line in the Modified contents.
+ std::map<IncludeKey, int> IncludeToPatchedLine;
if (IncludesChanged && PatchType == PatchType::All) {
// We are only interested in newly added includes, record the ones in
// Baseline for exclusion.
- llvm::DenseMap<std::pair<tok::PPKeywordKind, llvm::StringRef>,
- /*Resolved=*/llvm::StringRef>
- ExistingIncludes;
- for (const auto &Inc : Baseline.Includes.MainFileIncludes)
- ExistingIncludes[{Inc.Directive, Inc.Written}] = Inc.Resolved;
- // There might be includes coming from disabled regions, record these for
- // exclusion too. note that we don't have resolved paths for those.
- for (const auto &Inc : BaselineScan->Includes)
- ExistingIncludes.try_emplace({Inc.Directive, Inc.Written});
+ auto ExistingIncludes =
+ getExistingIncludes(*BaselineScan, Baseline.Includes.MainFileIncludes);
// Calculate extra includes that needs to be inserted.
for (auto &Inc : ModifiedScan->Includes) {
auto It = ExistingIncludes.find({Inc.Directive, Inc.Written});
// Include already present in the baseline preamble. Set resolved path and
// put into preamble includes.
if (It != ExistingIncludes.end()) {
- Inc.Resolved = It->second.str();
+ if (It->second) {
+ // Copy everything from existing include, apart from the location.
+ Inc.Resolved = It->second->Resolved;
+ Inc.HeaderID = It->second->HeaderID;
+ Inc.BehindPragmaKeep = It->second->BehindPragmaKeep;
+ Inc.FileKind = It->second->FileKind;
+ }
PP.PreambleIncludes.push_back(Inc);
+ IncludeToPatchedLine[It->first] = Inc.HashLine;
continue;
}
// Include is new in the modified preamble. Inject it into the patch and
@@ -694,8 +785,29 @@
Patch << llvm::formatv(
"#{0} {1}\n", spellingForIncDirective(Inc.Directive), Inc.Written);
}
+ } else {
+ // Copy include information from baseline preamble if nothing has changed.
+ PP.PreambleIncludes = Baseline.Includes.MainFileIncludes;
+ }
+
+ // Patch locations for diagnostics on baseline includes.
+ for (const auto &Inc : Baseline.Includes.MainFileIncludes) {
+ // Use existing locations if includes haven't changed.
+ size_t NewLine = Inc.HashLine;
+ if (IncludesChanged) {
+ auto PatchedLine =
+ IncludeToPatchedLine.find({Inc.Directive, Inc.Written});
+ // Ignore diags for deleted includes.
+ if (PatchedLine == IncludeToPatchedLine.end())
+ continue;
+ NewLine = PatchedLine->second;
+ }
+ MoveDiagsBetweenLines(Inc.HashLine, NewLine);
}
+ // Maps a directive from its spelling to its location in the Modified
+ // contents.
+ llvm::StringMap<int> DirectiveToPatchedLine;
if (DirectivesChanged) {
// We need to patch all the directives, since they are order dependent. e.g:
// #define BAR(X) NEW(X) // Newly introduced in Modified
@@ -715,8 +827,23 @@
Patch << "#undef " << TD.MacroName << '\n';
Patch << "#line " << TD.DirectiveLine << '\n';
Patch << TD.Text << '\n';
+ DirectiveToPatchedLine[TD.Text] = TD.DirectiveLine;
}
+ } else {
+ // Take existing directives as-is, if they're the same.
+ for (const auto &TD : BaselineScan->TextualDirectives)
+ DirectiveToPatchedLine[TD.Text] = TD.DirectiveLine;
}
+
+ // Patch locations for diagnositcs on baseline macros.
+ for (const auto &TD : BaselineScan->TextualDirectives) {
+ // Textual directive lines are 1-based.
+ auto NewLine = DirectiveToPatchedLine.find(TD.Text);
+ if (NewLine == DirectiveToPatchedLine.end())
+ continue;
+ MoveDiagsBetweenLines(TD.DirectiveLine - 1, NewLine->second - 1);
+ }
+
dlog("Created preamble patch: {0}", Patch.str());
Patch.flush();
return PP;
@@ -758,6 +885,7 @@
PreamblePatch PP;
PP.PreambleIncludes = Preamble.Includes.MainFileIncludes;
PP.ModifiedBounds = Preamble.Preamble.getBounds();
+ PP.PatchedDiags = Preamble.Diags;
return PP;
}
Index: clang-tools-extra/clangd/ParsedAST.cpp
===================================================================
--- clang-tools-extra/clangd/ParsedAST.cpp
+++ clang-tools-extra/clangd/ParsedAST.cpp
@@ -674,9 +674,10 @@
if (PreserveDiags) {
Diags = CompilerInvocationDiags;
// Add diagnostics from the preamble, if any.
- if (Preamble)
- Diags->insert(Diags->end(), Preamble->Diags.begin(),
- Preamble->Diags.end());
+ if (Preamble) {
+ auto PDiags = Patch->patchedDiags();
+ Diags->insert(Diags->end(), PDiags.begin(), PDiags.end());
+ }
// Finally, add diagnostics coming from the AST.
{
std::vector<Diag> D = ASTDiags.take(&*CTContext);
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
