kadircet updated this revision to Diff 496900.
kadircet marked 12 inline comments as done.
kadircet added a comment.
- Change patching logic to iterate over diags instead of preamble lines
- Change translation logic to:
- Preserve a diagnostic if its range can be translated.
- Preserve all the notes whose range can be translated.
- Preserve all the fixes whose edit ranges can be translated.
- Drop restrictions on ranges being a single line, make sure contents for all
the lines are matched between modified and basline contents.
- Add couple new tests to demonstrate what's preserved.
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
@@ -724,9 +724,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.
@@ -735,18 +734,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.
@@ -757,18 +753,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()))));
+ }
+ {
+ // Note is also dropped if diag is gone.
+ 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,8 @@
/// 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 +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 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,11 @@
#include "llvm/Support/Path.h"
#include "llvm/Support/VirtualFileSystem.h"
#include "llvm/Support/raw_ostream.h"
-#include <iterator>
+#include <cstddef>
+#include <cstdlib>
+#include <map>
#include <memory>
+#include <optional>
#include <string>
#include <system_error>
#include <utility>
@@ -306,6 +314,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 +342,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 +373,7 @@
return std::move(Err);
Action.EndSourceFile();
SP.Includes = std::move(Includes.MainFileIncludes);
+ llvm::append_range(SP.Lines, llvm::split(Contents, "\n"));
return SP;
}
@@ -612,6 +623,113 @@
}
}
+// 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;
+ llvm::StringMap<llvm::SmallVector<int>> ModifiedContentsToLine;
+ for (int Line = 0, E = ModifiedScan.Lines.size(); Line != E; ++Line) {
+ llvm::StringRef Contents = ModifiedScan.Lines[Line];
+ ModifiedContentsToLine[Contents].push_back(Line);
+ }
+ // Translates a range from baseline preamble contents to modified preamble
+ // contents.
+ // Finds the consecutive set of lines that corresponds to the same contents in
+ // baseline and modified, and applies the same translation to the range.
+ // Returns nullopt if no exact line-based match is found.
+ auto TranslateRange = [&](const Range &R) -> std::optional<Range> {
+ int BaselineStart = R.start.line;
+ int BaselineEnd = R.end.line;
+ auto ModifiedStartIt =
+ ModifiedContentsToLine.find(BaselineScan.Lines[BaselineStart]);
+ if (ModifiedStartIt == ModifiedContentsToLine.end())
+ return std::nullopt;
+ int Closest = ModifiedStartIt->second.front();
+ for (auto AlternateLine : ModifiedStartIt->second) {
+ if (abs(AlternateLine - BaselineStart) < abs(Closest - BaselineStart))
+ Closest = AlternateLine;
+ }
+ for (int I = 1; I <= BaselineEnd - BaselineStart; ++I) {
+ if (BaselineScan.Lines[BaselineStart + I] !=
+ ModifiedScan.Lines[Closest + I])
+ return std::nullopt;
+ }
+ Range NewRange = R;
+ NewRange.start.line = Closest;
+ NewRange.end.line = Closest + BaselineEnd - BaselineStart;
+ return NewRange;
+ };
+ // Translates a Note by patching its range when inside main file.
+ auto TranslateNote = [&](const Note &N) -> std::optional<Note> {
+ if (!N.InsideMainFile)
+ return N;
+ if (auto NewRange = TranslateRange(N.Range)) {
+ Note NewN = N;
+ NewN.Range = std::move(*NewRange);
+ return NewN;
+ }
+ return std::nullopt;
+ };
+ // Tries to translate all the edit ranges inside the fix. Returns a fix with
+ // translated ranges on success.
+ auto TranslateFix = [&](const Fix &F) -> std::optional<Fix> {
+ Fix NewFix;
+ for (auto &E : F.Edits) {
+ auto NewRange = TranslateRange(E.range);
+ if (!NewRange.has_value())
+ return std::nullopt;
+ NewFix.Edits.emplace_back(E);
+ NewFix.Edits.back().range = *NewRange;
+ }
+ NewFix.Message = F.Message;
+ return NewFix;
+ };
+ // 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
+ // modified contents.
+ // Drops the whole diagnostic if main range can't be patched.
+ auto TranslateDiag = [&](const Diag &D) -> std::optional<Diag> {
+ Range NewRange;
+ // Patch range if it's inside main file.
+ if (D.InsideMainFile) {
+ auto PatchedRange = TranslateRange(D.Range);
+ // Drop the diagnostic if we couldn't patch the range.
+ if (!PatchedRange.has_value())
+ return std::nullopt;
+ NewRange = *PatchedRange;
+ } else {
+ NewRange = D.Range;
+ }
+
+ Diag NewDiag;
+ // Copy all fields but Notes, Fixes, Name and Tags.
+ static_cast<DiagBase &>(NewDiag) = static_cast<DiagBase>(D);
+ NewDiag.Range = NewRange;
+ // Only keep Notes and Fixes that are still relevant.
+ for (auto &N : D.Notes) {
+ if (auto NewN = TranslateNote(N))
+ NewDiag.Notes.emplace_back(std::move(*NewN));
+ }
+ for (auto &F : D.Fixes) {
+ if (auto NewF = TranslateFix(F))
+ NewDiag.Fixes.emplace_back(std::move(*NewF));
+ }
+ // Name and Tags are not positional.
+ NewDiag.Name = D.Name;
+ NewDiag.Tags = D.Tags;
+ return NewDiag;
+ };
+ for (auto &D : BaselineDiags) {
+ if (auto NewD = TranslateDiag(D))
+ PatchedDiags.emplace_back(std::move(*NewD));
+ }
+ return PatchedDiags;
+}
+
PreamblePatch PreamblePatch::create(llvm::StringRef FileName,
const ParseInputs &Modified,
const PreambleData &Baseline,
@@ -619,9 +737,10 @@
trace::Span Tracer("CreatePreamblePatch");
SPAN_ATTACH(Tracer, "File", FileName);
assert(llvm::sys::path::is_absolute(FileName) && "relative FileName!");
- // First scan preprocessor directives in Baseline and Modified. These will be
- // used to figure out newly added directives in Modified. Scanning can fail,
- // the code just bails out and creates an empty patch in such cases, as:
+ // First scan preprocessor directives in Baseline and Modified. These will
+ // be used to figure out newly added directives in Modified. Scanning can
+ // fail, the code just bails out and creates an empty patch in such cases,
+ // as:
// - If scanning for Baseline fails, no knowledge of existing includes hence
// patch will contain all the includes in Modified. Leading to rebuild of
// whole preamble, which is terribly slow.
@@ -629,7 +748,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());
@@ -665,6 +784,7 @@
escapeBackslashAndQuotes(FileName, Patch);
Patch << "\"\n";
+ // Map from an include to its line in the Modified contents.
if (IncludesChanged && PatchType == PatchType::All) {
// We are only interested in newly added includes, record the ones in
// Baseline for exclusion.
@@ -680,8 +800,8 @@
// 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.
+ // Include already present in the baseline preamble. Set resolved path
+ // and put into preamble includes.
if (It != ExistingIncludes.end()) {
if (It->second) {
// If this header is included in an active region of the baseline
@@ -703,20 +823,20 @@
"#{0} {1}\n", spellingForIncDirective(Inc.Directive), Inc.Written);
}
} else {
- // Make sure we have the full set of includes available even when we're not
- // patching. As these are used by features we provide afterwards like hover,
- // go-to-def or include-cleaner when preamble is stale.
+ // Make sure we have the full set of includes available even when we're
+ // not patching. As these are used by features we provide afterwards like
+ // hover, go-to-def or include-cleaner when preamble is stale.
PP.PreambleIncludes = Baseline.Includes.MainFileIncludes;
}
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
- // #define BAR(X) OLD(X) // Exists in the Baseline
+ // We need to patch all the directives, since they are order dependent.
+ // e.g: #define BAR(X) NEW(X) // Newly introduced in Modified #define
+ // BAR(X) OLD(X) // Exists in the Baseline
//
- // If we've patched only the first directive, the macro definition would've
- // been wrong for the rest of the file, since patch is applied after the
- // baseline preamble.
+ // If we've patched only the first directive, the macro definition
+ // would've been wrong for the rest of the file, since patch is applied
+ // after the baseline preamble.
//
// Note that we deliberately ignore conditional directives and undefs to
// reduce complexity. The former might cause problems because scanning is
@@ -730,6 +850,8 @@
Patch << TD.Text << '\n';
}
}
+
+ PP.PatchedDiags = patchDiags(Baseline.Diags, *BaselineScan, *ModifiedScan);
dlog("Created preamble patch: {0}", Patch.str());
Patch.flush();
return PP;
@@ -758,8 +880,8 @@
llvm::MemoryBuffer::getMemBufferCopy(PatchContents, PatchFileName);
// CI will take care of the lifetime of the buffer.
PPOpts.addRemappedFile(PatchFileName, PatchBuffer.release());
- // The patch will be parsed after loading the preamble ast and before parsing
- // the main file.
+ // The patch will be parsed after loading the preamble ast and before
+ // parsing the main file.
PPOpts.Includes.push_back(PatchFileName);
}
@@ -771,6 +893,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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
