kadircet updated this revision to Diff 498308.
kadircet marked 6 inline comments as done.
kadircet added a comment.
- Move patch translation logic to a separate class
- Perform eager copies and use `bool translateX(X&)` type of APIs instead of
returning optionals.
- Perform multi line content check for each alternative, not just the closest
match.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143096/new/
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
@@ -723,9 +723,8 @@
#define BAR
#include [[<foo>]])");
auto AST = createPatchedAST(Code.code(), NewCode.code());
- // FIXME: We should point at the correct coordinates in NewCode.
EXPECT_THAT(*AST->getDiagnostics(),
- ElementsAre(Diag(Code.range(), "pp_file_not_found")));
+ ElementsAre(Diag(NewCode.range(), "pp_file_not_found")));
}
{
// Check with removals from preamble.
@@ -734,18 +733,15 @@
#include [[<foo>]])");
Annotations NewCode("#include [[<foo>]]");
auto AST = createPatchedAST(Code.code(), NewCode.code());
- // FIXME: We should point at the correct coordinates in NewCode.
EXPECT_THAT(*AST->getDiagnostics(),
- ElementsAre(Diag(Code.range(), "pp_file_not_found")));
+ ElementsAre(Diag(NewCode.range(), "pp_file_not_found")));
}
{
// Drop line with diags.
Annotations Code("#include [[<foo>]]");
Annotations NewCode("#define BAR\n#define BAZ\n");
auto AST = createPatchedAST(Code.code(), NewCode.code());
- // FIXME: No diagnostics.
- EXPECT_THAT(*AST->getDiagnostics(),
- ElementsAre(Diag(Code.range(), "pp_file_not_found")));
+ EXPECT_THAT(*AST->getDiagnostics(), IsEmpty());
}
{
// Picks closest line in case of multiple alternatives.
@@ -756,18 +752,79 @@
#define BAR
#include <foo>)");
auto AST = createPatchedAST(Code.code(), NewCode.code());
- // FIXME: We should point at the correct coordinates in NewCode.
EXPECT_THAT(*AST->getDiagnostics(),
- ElementsAre(Diag(Code.range(), "pp_file_not_found")));
+ ElementsAre(Diag(NewCode.range(), "pp_file_not_found")));
}
{
// Drop diag if line spelling has changed.
Annotations Code("#include [[<foo>]]");
Annotations NewCode(" # include <foo>");
auto AST = createPatchedAST(Code.code(), NewCode.code());
- // FIXME: No diags.
+ EXPECT_THAT(*AST->getDiagnostics(), IsEmpty());
+ }
+ {
+ // Multiple lines.
+ Annotations Code(R"(
+#define BAR
+#include [[<fo\
+o>]])");
+ Annotations NewCode(R"(#include [[<fo\
+o>]])");
+ auto AST = createPatchedAST(Code.code(), NewCode.code());
EXPECT_THAT(*AST->getDiagnostics(),
- ElementsAre(Diag(Code.range(), "pp_file_not_found")));
+ ElementsAre(Diag(NewCode.range(), "pp_file_not_found")));
+ }
+ {
+ // Multiple lines with change.
+ Annotations Code(R"(
+#define BAR
+#include <fox>
+#include [[<fo\
+o>]])");
+ Annotations NewCode(R"(#include <fo\
+x>)");
+ auto AST = createPatchedAST(Code.code(), NewCode.code());
+ EXPECT_THAT(*AST->getDiagnostics(), IsEmpty());
+ }
+ {
+ // Preserves notes.
+ Annotations Code(R"(
+#define $note[[BAR]] 1
+#define $main[[BAR]] 2)");
+ Annotations NewCode(R"(
+#define BAZ 0
+#define $note[[BAR]] 1
+#define BAZ 0
+#define $main[[BAR]] 2)");
+ auto AST = createPatchedAST(Code.code(), NewCode.code());
+ EXPECT_THAT(
+ *AST->getDiagnostics(),
+ ElementsAre(AllOf(Diag(NewCode.range("main"), "-Wmacro-redefined"),
+ withNote(Diag(NewCode.range("note"))))));
+ }
+ {
+ // Preserves diag without note.
+ Annotations Code(R"(
+#define $note[[BAR]] 1
+#define $main[[BAR]] 2)");
+ Annotations NewCode(R"(
+#define $main[[BAR]] 2)");
+ auto AST = createPatchedAST(Code.code(), NewCode.code());
+ EXPECT_THAT(
+ *AST->getDiagnostics(),
+ ElementsAre(AllOf(Diag(NewCode.range("main"), "-Wmacro-redefined"),
+ Field(&Diag::Notes, IsEmpty()))));
+ }
+ {
+ // Make sure orphaned notes are not promoted to diags.
+ Annotations Code(R"(
+#define $note[[BAR]] 1
+#define $main[[BAR]] 2)");
+ Annotations NewCode(R"(
+#define BAZ 0
+#define BAR 1)");
+ auto AST = createPatchedAST(Code.code(), NewCode.code());
+ EXPECT_THAT(*AST->getDiagnostics(), IsEmpty());
}
}
} // namespace
Index: clang-tools-extra/clangd/Preamble.h
===================================================================
--- clang-tools-extra/clangd/Preamble.h
+++ clang-tools-extra/clangd/Preamble.h
@@ -34,6 +34,7 @@
#include "clang/Frontend/PrecompiledPreamble.h"
#include "clang/Lex/Lexer.h"
#include "clang/Tooling/CompilationDatabase.h"
+#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/StringRef.h"
#include <memory>
@@ -157,6 +158,9 @@
/// Whether diagnostics generated using this patch are trustable.
bool preserveDiagnostics() const;
+ /// Returns diag locations for Modified contents.
+ llvm::ArrayRef<Diag> patchedDiags() const { return PatchedDiags; }
+
static constexpr llvm::StringLiteral HeaderName = "__preamble_patch__.h";
private:
@@ -168,9 +172,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 Baseline and Modified. Used for
+ // patching includes of baseline preamble.
std::vector<Inclusion> PreambleIncludes;
+ // Diags that were attached to a line preserved in Modified contents.
+ 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,7 +9,9 @@
#include "Preamble.h"
#include "Compiler.h"
#include "Config.h"
+#include "Diagnostics.h"
#include "Headers.h"
+#include "Protocol.h"
#include "SourceCode.h"
#include "clang-include-cleaner/Record.h"
#include "support/Logger.h"
@@ -35,6 +37,9 @@
#include "llvm/ADT/IntrusiveRefCntPtr.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/ErrorHandling.h"
@@ -43,8 +48,9 @@
#include "llvm/Support/Path.h"
#include "llvm/Support/VirtualFileSystem.h"
#include "llvm/Support/raw_ostream.h"
-#include <iterator>
+#include <cstddef>
#include <memory>
+#include <optional>
#include <string>
#include <system_error>
#include <utility>
@@ -306,6 +312,8 @@
struct ScannedPreamble {
std::vector<Inclusion> Includes;
std::vector<TextualPPDirective> TextualDirectives;
+ // Literal lines of the preamble contents.
+ std::vector<llvm::StringRef> Lines;
PreambleBounds Bounds = {0, false};
};
@@ -332,7 +340,7 @@
if (!CI)
return error("failed to create compiler invocation");
CI->getDiagnosticOpts().IgnoreWarnings = true;
- auto ContentsBuffer = llvm::MemoryBuffer::getMemBuffer(Contents);
+ auto ContentsBuffer = llvm::MemoryBuffer::getMemBuffer(PI.Contents);
// This means we're scanning (though not preprocessing) the preamble section
// twice. However, it's important to precisely follow the preamble bounds used
// elsewhere.
@@ -363,6 +371,7 @@
return std::move(Err);
Action.EndSourceFile();
SP.Includes = std::move(Includes.MainFileIncludes);
+ llvm::append_range(SP.Lines, llvm::split(Contents, "\n"));
return SP;
}
@@ -465,6 +474,106 @@
WallTimer Timer;
};
+// Helpers for patching diagnostics between two versions of file contents.
+class DiagPatcher {
+ llvm::ArrayRef<llvm::StringRef> OldLines;
+ llvm::ArrayRef<llvm::StringRef> CurrentLines;
+ llvm::StringMap<llvm::SmallVector<int>> CurrentContentsToLine;
+
+ // Translates a range from old lines to current lines.
+ // Finds the consecutive set of lines that corresponds to the same contents in
+ // old and current, and applies the same translation to the range.
+ // Returns true if translation succeeded.
+ bool translateRange(Range &R) {
+ int OldStart = R.start.line;
+ int OldEnd = R.end.line;
+ assert(0 <= OldStart && OldStart <= OldEnd);
+ // By definition preamble bounds start from the beginning of the file, so if
+ // Range's start is beyond OldLines, range is not within the preamble.
+ if ((size_t)OldEnd >= OldLines.size())
+ return false;
+ auto CurrentStartIt = CurrentContentsToLine.find(OldLines[OldStart]);
+ if (CurrentStartIt == CurrentContentsToLine.end())
+ return false;
+
+ size_t RangeLen = OldEnd - OldStart + 1;
+ auto RangeContents = OldLines.slice(OldStart, RangeLen);
+ int Closest = -1;
+ for (int AlternateLine : CurrentStartIt->second) {
+ size_t CurrentEnd = AlternateLine + RangeLen;
+ // Don't consider this as an alternative if end doesn't fall in
+ // CurrentLines.
+ if (CurrentEnd > CurrentLines.size())
+ continue;
+ // Check if AlternateLine matches all lines in the range.
+ if (llvm::any_of(
+ llvm::zip_equal(RangeContents,
+ CurrentLines.slice(AlternateLine, RangeLen)),
+ [](const auto &Lines) {
+ return std::get<0>(Lines) != std::get<1>(Lines);
+ }))
+ continue;
+
+ if (Closest == -1 ||
+ abs(AlternateLine - OldStart) < abs(Closest - OldStart))
+ Closest = AlternateLine;
+ }
+ // Couldn't find any viable matches in the current contents.
+ if (Closest == -1)
+ return false;
+ R.start.line = Closest;
+ R.end.line = Closest + RangeLen - 1; // -1 because end is inclusive.
+ return true;
+ }
+
+ // Translates a Note by patching its range when inside main file. Returns true
+ // on success.
+ bool translateNote(Note &N) {
+ if (!N.InsideMainFile)
+ return true;
+ if (translateRange(N.Range))
+ return true;
+ return false;
+ }
+
+ // Tries to translate all the edit ranges inside the fix. Returns true on
+ // success. On failure fixes might be in an invalid state.
+ bool translateFix(Fix &F) {
+ return llvm::all_of(
+ F.Edits, [this](TextEdit &E) { return translateRange(E.range); });
+ }
+
+public:
+ DiagPatcher(llvm::ArrayRef<llvm::StringRef> OldLines,
+ llvm::ArrayRef<llvm::StringRef> CurrentLines) {
+ this->OldLines = OldLines;
+ this->CurrentLines = CurrentLines;
+ for (int Line = 0, E = CurrentLines.size(); Line != E; ++Line) {
+ llvm::StringRef Contents = CurrentLines[Line];
+ CurrentContentsToLine[Contents].push_back(Line);
+ }
+ }
+ // Translate diagnostic by moving its main range to new location (if inside
+ // the main file). Preserve all the notes and fixes that can be translated to
+ // new contents.
+ // Drops the whole diagnostic if main range can't be patched.
+ std::optional<Diag> translateDiag(const Diag &D) {
+ Range NewRange = D.Range;
+ // Patch range if it's inside main file.
+ if (D.InsideMainFile && !translateRange(NewRange)) {
+ // Drop the diagnostic if we couldn't patch the range.
+ return std::nullopt;
+ }
+
+ Diag NewD = D;
+ NewD.Range = NewRange;
+ // Translate ranges inside notes and fixes too, dropping the ones that are
+ // no longer relevant.
+ llvm::erase_if(NewD.Notes, [this](Note &N) { return !translateNote(N); });
+ llvm::erase_if(NewD.Fixes, [this](Fix &F) { return !translateFix(F); });
+ return NewD;
+ }
+};
} // namespace
std::shared_ptr<const PreambleData>
@@ -612,6 +721,22 @@
}
}
+// Translate diagnostics from baseline into modified for the lines that have the
+// same spelling.
+static std::vector<Diag> patchDiags(llvm::ArrayRef<Diag> BaselineDiags,
+ const ScannedPreamble &BaselineScan,
+ const ScannedPreamble &ModifiedScan) {
+ std::vector<Diag> PatchedDiags;
+ if (BaselineDiags.empty())
+ return PatchedDiags;
+ DiagPatcher Patcher(BaselineScan.Lines, ModifiedScan.Lines);
+ for (auto &D : BaselineDiags) {
+ if (auto NewD = Patcher.translateDiag(D))
+ PatchedDiags.emplace_back(std::move(*NewD));
+ }
+ return PatchedDiags;
+}
+
PreamblePatch PreamblePatch::create(llvm::StringRef FileName,
const ParseInputs &Modified,
const PreambleData &Baseline,
@@ -629,7 +754,7 @@
// there's nothing to do but generate an empty patch.
auto BaselineScan = scanPreamble(
// Contents needs to be null-terminated.
- Baseline.Preamble.getContents().str(), Modified.CompileCommand);
+ Baseline.Preamble.getContents(), Modified.CompileCommand);
if (!BaselineScan) {
elog("Failed to scan baseline of {0}: {1}", FileName,
BaselineScan.takeError());
@@ -730,6 +855,8 @@
Patch << TD.Text << '\n';
}
}
+
+ PP.PatchedDiags = patchDiags(Baseline.Diags, *BaselineScan, *ModifiedScan);
dlog("Created preamble patch: {0}", Patch.str());
Patch.flush();
return PP;
@@ -771,6 +898,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
@@ -675,8 +675,7 @@
Diags = CompilerInvocationDiags;
// Add diagnostics from the preamble, if any.
if (Preamble)
- Diags->insert(Diags->end(), Preamble->Diags.begin(),
- Preamble->Diags.end());
+ llvm::append_range(*Diags, Patch->patchedDiags());
// Finally, add diagnostics coming from the AST.
{
std::vector<Diag> D = ASTDiags.take(&*CTContext);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
