kadircet updated this revision to Diff 495411.
kadircet added a comment.
- Change to a line based translation logic
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
@@ -696,24 +696,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#define BAZ\n");
auto AST = createPatchedAST(Code.code(), NewCode.code());
- // FIXME: No diagnostics.
- EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(Code.range()));
+ EXPECT_THAT(mainFileDiagRanges(*AST), IsEmpty());
}
{
// Picks closest line in case of multiple alternatives.
@@ -724,16 +721,14 @@
#define BAR
#include <foo>)");
auto AST = createPatchedAST(Code.code(), NewCode.code());
- // FIXME: Should point at ranges in NewCode.
- EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(Code.range()));
+ EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(NewCode.range()));
}
{
// Drop diag if line spelling has changed.
Annotations Code(BaselinePreamble);
Annotations NewCode(" # include <foo>");
auto AST = createPatchedAST(Code.code(), NewCode.code());
- // FIXME: No diags.
- EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(Code.range()));
+ EXPECT_THAT(mainFileDiagRanges(*AST), 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, only contains diags attached
+ /// to an #include or #define directive.
+ 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 \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,9 @@
#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 <string>
#include <system_error>
@@ -306,6 +311,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 +339,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 +370,10 @@
return std::move(Err);
Action.EndSourceFile();
SP.Includes = std::move(Includes.MainFileIncludes);
+ for (auto Pos = Contents.find('\n'); Pos != Contents.npos;
+ Contents = Contents.substr(Pos + 1), Pos = Contents.find('\n')) {
+ SP.Lines.push_back(Contents.substr(0, Pos));
+ }
return SP;
}
@@ -465,6 +476,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>
@@ -558,8 +582,8 @@
if (BuiltPreamble) {
log("Built preamble of size {0} for file {1} version {2} in {3} seconds",
- BuiltPreamble->getSize(), FileName, Inputs.Version,
- PreambleTimer.getTime());
+ BuiltPreamble->getSize(), FileName, Inputs.Version,
+ PreambleTimer.getTime());
std::vector<Diag> Diags = PreambleDiagnostics.take();
auto Result = std::make_shared<PreambleData>(std::move(*BuiltPreamble));
Result->Version = Inputs.Version;
@@ -612,6 +636,105 @@
}
}
+// 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;
+}
+
+// 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;
+ // List of diagnostics associated to a particular line in the baseline.
+ auto LineToDiags = mapDiagsToLines(BaselineDiags);
+ auto MoveDiagsBetweenLines = [&LineToDiags, &PatchedDiags](int OldLine,
+ int NewLine) {
+ auto DiagsForInclude = LineToDiags.find(OldLine);
+ if (DiagsForInclude == LineToDiags.end())
+ return;
+ for (auto *D : DiagsForInclude->second)
+ PatchedDiags.emplace_back(translateDiag(*D, NewLine));
+ };
+ llvm::StringMap<llvm::SmallVector<int>> BaselineContentsToLine;
+ for (int Line = 0, E = BaselineScan.Lines.size(); Line != E; ++Line) {
+ llvm::StringRef Contents = BaselineScan.Lines[Line];
+ BaselineContentsToLine[Contents].push_back(Line);
+ }
+ for (int Line = 0, E = ModifiedScan.Lines.size(); Line != E; ++Line) {
+ auto PreviousLine = BaselineContentsToLine.find(ModifiedScan.Lines[Line]);
+ if (PreviousLine == BaselineContentsToLine.end())
+ continue;
+ int Closest = PreviousLine->second.front();
+ for (auto AlternateLine : PreviousLine->second) {
+ if (abs(AlternateLine - Line) < abs(Closest - Line))
+ Closest = AlternateLine;
+ }
+ MoveDiagsBetweenLines(Closest, Line);
+ }
+ return PatchedDiags;
+}
+
PreamblePatch PreamblePatch::create(llvm::StringRef FileName,
const ParseInputs &Modified,
const PreambleData &Baseline,
@@ -629,7 +752,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,18 +788,12 @@
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.
- llvm::DenseMap<std::pair<tok::PPKeywordKind, llvm::StringRef>,
- const Inclusion *>
- ExistingIncludes;
- for (const auto &Inc : Baseline.Includes.MainFileIncludes)
- ExistingIncludes[{Inc.Directive, Inc.Written}] = &Inc;
- // 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});
@@ -728,6 +845,8 @@
Patch << TD.Text << '\n';
}
}
+
+ PP.PatchedDiags = patchDiags(Baseline.Diags, *BaselineScan, *ModifiedScan);
dlog("Created preamble patch: {0}", Patch.str());
Patch.flush();
return PP;
@@ -769,6 +888,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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
