[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

thanks for the ping! see the discussion in 
https://reviews.llvm.org/D142890#4149679


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143096/new/

https://reviews.llvm.org/D143096

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

[Mikael Holmén via Phabricator via cfe-commits]

uabelho added a comment.

Hello,

I noticed that if I compile the compiler with ubsan, then lots of Clangd tests 
start failing with this patch:

  Failed Tests (165):
Clangd :: ast-no-range.test
Clangd :: ast.test
Clangd :: call-hierarchy.test
Clangd :: check-lines.test
Clangd :: check.test
Clangd :: code-action-request.test
Clangd :: completion-auto-trigger.test
Clangd :: completion-snippets.test
Clangd :: completion.test
Clangd :: config.test
Clangd :: crash-parse.test
Clangd :: dependency-output.test
Clangd :: diagnostic-category.test
Clangd :: diagnostics-no-tidy.test
Clangd :: diagnostics-notes.test
Clangd :: diagnostics-tidy.test
Clangd :: did-change-configuration-params.test
Clangd :: execute-command.test
Clangd :: filestatus.test
Clangd :: fixits-codeaction.test
Clangd :: fixits-command.test
Clangd :: fixits-embed-in-diagnostic.test
Clangd :: folding-range.test
Clangd :: formatting.test
Clangd :: hover.test
Clangd :: implementations.test
Clangd :: inlayHints.test
Clangd :: memory_tree.test
Clangd :: metrics.test
Clangd :: protocol.test
Clangd :: references-container.test
Clangd :: references.test
Clangd :: rename.test
Clangd :: request-reply.test
Clangd :: selection-range.test
Clangd :: semantic-tokens-refresh.test
Clangd :: semantic-tokens.test
Clangd :: signature-help-with-offsets.test
Clangd :: signature-help.test
Clangd :: symbol-info.test
Clangd :: symbols.test
Clangd :: target_info.test
Clangd :: test-uri-posix.test
Clangd :: textdocument-didchange-fail.test
Clangd :: trace.test
Clangd :: tweaks-format.test
Clangd :: type-definition.test
Clangd :: type-hierarchy-ext.test
Clangd :: type-hierarchy.test
Clangd :: unsupported-method.test
Clangd :: utf8.test
Clangd :: version.test
Clangd :: xrefs.test
Clangd Unit Tests :: ./ClangdTests/0/146
[...]
Clangd Unit Tests :: ./ClangdTests/99/146
  
  
  Testing Time: 362.21s
Skipped  :47
Unsupported  :  1094
Passed   : 89208
Expectedly Failed:   192
Failed   :   165

Not sure if all fail the same way but there are a lot of ubsan complaints like 
this

  07:38:09 ../lib/Support/MemoryBuffer.cpp:138:33: runtime error: null pointer 
passed as argument 2, which is declared to never be null
  07:38:09 /usr/include/string.h:43:28: note: nonnull attribute specified here
  07:38:09 SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior 
../lib/Support/MemoryBuffer.cpp:138:33 in 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143096/new/

https://reviews.llvm.org/D143096

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8c2a12f7f9b6: [clangd] Provide patched diagnostics with 
preamble patch (authored by kadircet).

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 [[]])");
 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 [[]])");
 Annotations NewCode("#include [[]]");
 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 [[]]");
 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 )");
 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 [[]]");
 Annotations NewCode(" # include ");
 auto AST = createPatchedAST(Code.code(), NewCode.code());
-// FIXME: No diags.
+EXPECT_THAT(*AST->getDiagnostics(), IsEmpty());
+  }
+  {
+// Multiple lines.
+Annotations Code(R"(
+#define BAR
+#include [[]])");
+Annotations NewCode(R"(#include [[]])");
+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 
+#include [[]])");
+Annotations NewCode(R"(#include )");
+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

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

thanks for the review!




Comment at: clang-tools-extra/clangd/Preamble.cpp:509
+  // Check if AlternateLine matches all lines in the range.
+  if (llvm::any_of(
+  llvm::zip_equal(RangeContents,

sammccall wrote:
> why not just `if (RangeContents != CurrentLines.slice(...))`? I feel like I'm 
> missing something subtle :-)
> 
> Also if you replace the `slice(a, b)` with `drop_front(a).take_front(b)` then 
> you don't need the `CurrentEnd > CurrentLines.size()` check
> why not just if (RangeContents != CurrentLines.slice(...))? I feel like I'm 
> missing something subtle :-)

no that's absolutely right, i guess i had a brain fart 😂 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143096/new/

https://reviews.llvm.org/D143096

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 498351.
kadircet marked 4 inline comments as done.
kadircet added a comment.

- Use direct comparison of ArrayRefs
- optional for closest
- loop over lookup results rather than through iterator


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 [[]])");
 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 [[]])");
 Annotations NewCode("#include [[]]");
 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 [[]]");
 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 )");
 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 [[]]");
 Annotations NewCode(" # include ");
 auto AST = createPatchedAST(Code.code(), NewCode.code());
-// FIXME: No diags.
+EXPECT_THAT(*AST->getDiagnostics(), IsEmpty());
+  }
+  {
+// Multiple lines.
+Annotations Code(R"(
+#define BAR
+#include [[]])");
+Annotations NewCode(R"(#include [[]])");
+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 
+#include [[]])");
+Annotations NewCode(R"(#include )");
+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/Compil

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

LG thanks!




Comment at: clang-tools-extra/clangd/Preamble.cpp:496
+auto CurrentStartIt = CurrentContentsToLine.find(OldLines[OldStart]);
+if (CurrentStartIt == CurrentContentsToLine.end())
+  return false;

up to you, but I find avoiding iterators easier to read

`for (int AlternateLine : CurrentContentsToLine.lookup(OldLines[OldStart]))`

(or with extracted variables)



Comment at: clang-tools-extra/clangd/Preamble.cpp:509
+  // Check if AlternateLine matches all lines in the range.
+  if (llvm::any_of(
+  llvm::zip_equal(RangeContents,

why not just `if (RangeContents != CurrentLines.slice(...))`? I feel like I'm 
missing something subtle :-)

Also if you replace the `slice(a, b)` with `drop_front(a).take_front(b)` then 
you don't need the `CurrentEnd > CurrentLines.size()` check



Comment at: clang-tools-extra/clangd/Preamble.cpp:517
+
+  if (Closest == -1 ||
+  abs(AlternateLine - OldStart) < abs(Closest - OldStart))

if you make `Closest` the delta rather than the line number this gets a bit 
easier to follow IMO:
`abs(Closest) < abs(Delta)` here
`R.start.line += Closest; R.end.line += Closest;` below

You need to make `Closest` an `optional` but that's also clearer than a 
sentinel -1 for me



Comment at: clang-tools-extra/clangd/Preamble.cpp:568
+
+Diag NewD = D;
+NewD.Range = NewRange;

FWIW if we're willing to pay a copy for a successful patch, I don't think it's 
worth any hassle to avoid the copy for a failed patch. Up to you though.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143096/new/

https://reviews.llvm.org/D143096

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/Preamble.cpp:625
+// same spelling.
+static std::vector patchDiags(llvm::ArrayRef BaselineDiags,
+const ScannedPreamble &BaselineScan,

sammccall wrote:
> I think this function is too long with too many local lambdas, consider 
> converting it to a class instead (call me old-fashioned!)
well it's gonna be an equally long class, but sure :D



Comment at: clang-tools-extra/clangd/Preamble.cpp:645
+auto ModifiedStartIt =
+ModifiedContentsToLine.find(BaselineScan.Lines[BaselineStart]);
+if (ModifiedStartIt == ModifiedContentsToLine.end())

sammccall wrote:
> we're assuming that the ranges are within the preamble and everyone agrees 
> about the bounds. If not, BaselineStart may not be a valid index into 
> BaselineScan.Lines
> 
> This assumption seems broadly reasonable, but is *very much* not locally 
> enforced. I'd suggest a defensive check or at least an assert.
oops, thanks! both here and also the multi-ine check below.



Comment at: clang-tools-extra/clangd/Preamble.cpp:648
+  return std::nullopt;
+int Closest = ModifiedStartIt->second.front();
+for (auto AlternateLine : ModifiedStartIt->second) {

sammccall wrote:
> this doesn't look right: you're first deciding which possible starting point 
> is closest, and then deciding whether it matches. So a range that matches can 
> be masked by a range where only the first line matches, if the latter is 
> closer.
i thought because this would be rare it's fine to not do that, but the same 
applies in the other direction as well :D
moved the content based matching inside the loop to consider a line as 
alternative only after it matches the contents.



Comment at: clang-tools-extra/clangd/Preamble.cpp:706
+Diag NewDiag;
+// Copy all fields but Notes, Fixes, Name and Tags.
+static_cast(NewDiag) = static_cast(D);

sammccall wrote:
> reasoning about the fields you're *not* rewriting feels fragile. (Here and in 
> TranslateFix).
> 
> Consider copying the whole object and mutating in place (`bool 
> TranslateDiag(Diag&)` together with `erase_if`)
well i am a little worried about copying around notes/fixes just to drop them 
again (which can have quite some strings). but probably rare enough in practice 
to not matter.



Comment at: clang-tools-extra/clangd/unittests/PreambleTests.cpp:820
+  {
+// Note is also dropped if diag is gone.
+Annotations Code(R"(

sammccall wrote:
> i'm confused about what this comment is getting at - a note without a diag is 
> not even representable
well, it is to make sure we're not promoting an orphaned note to a main diag. 
updated the comment but happy to drop if you think it's confusing rather than 
being helpful.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143096/new/

https://reviews.llvm.org/D143096

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

[Kadir Cetinkaya via Phabricator via cfe-commits]

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 [[]])");
 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 [[]])");
 Annotations NewCode("#include [[]]");
 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 [[]]");
 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 )");
 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 [[]]");
 Annotations NewCode(" # include ");
 auto AST = createPatchedAST(Code.code(), NewCode.code());
-// FIXME: No diags.
+EXPECT_THAT(*AST->getDiagnostics(), IsEmpty());
+  }
+  {
+// Multiple lines.
+Annotations Code(R"(
+#define BAR
+#include [[]])");
+Annotations NewCode(R"(#include [[]])");
+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 
+#include [[]])");
+Annotations NewCode(R"(#include )");
+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

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/Preamble.cpp:625
+// same spelling.
+static std::vector patchDiags(llvm::ArrayRef BaselineDiags,
+const ScannedPreamble &BaselineScan,

I think this function is too long with too many local lambdas, consider 
converting it to a class instead (call me old-fashioned!)



Comment at: clang-tools-extra/clangd/Preamble.cpp:645
+auto ModifiedStartIt =
+ModifiedContentsToLine.find(BaselineScan.Lines[BaselineStart]);
+if (ModifiedStartIt == ModifiedContentsToLine.end())

we're assuming that the ranges are within the preamble and everyone agrees 
about the bounds. If not, BaselineStart may not be a valid index into 
BaselineScan.Lines

This assumption seems broadly reasonable, but is *very much* not locally 
enforced. I'd suggest a defensive check or at least an assert.



Comment at: clang-tools-extra/clangd/Preamble.cpp:648
+  return std::nullopt;
+int Closest = ModifiedStartIt->second.front();
+for (auto AlternateLine : ModifiedStartIt->second) {

this doesn't look right: you're first deciding which possible starting point is 
closest, and then deciding whether it matches. So a range that matches can be 
masked by a range where only the first line matches, if the latter is closer.



Comment at: clang-tools-extra/clangd/Preamble.cpp:683
+  NewFix.Edits.emplace_back(E);
+  NewFix.Edits.back().range = *NewRange;
+}

probably doesn't matter, but you're inconsistent about moving vs copying range



Comment at: clang-tools-extra/clangd/Preamble.cpp:706
+Diag NewDiag;
+// Copy all fields but Notes, Fixes, Name and Tags.
+static_cast(NewDiag) = static_cast(D);

reasoning about the fields you're *not* rewriting feels fragile. (Here and in 
TranslateFix).

Consider copying the whole object and mutating in place (`bool 
TranslateDiag(Diag&)` together with `erase_if`)



Comment at: clang-tools-extra/clangd/Preamble.h:162
+  /// Returns diag locations for Modified contents.
+  llvm::ArrayRef patchedDiags() const { return PatchedDiags; }
   static constexpr llvm::StringLiteral HeaderName = "__preamble_patch__.h";

nit: blank line after



Comment at: clang-tools-extra/clangd/unittests/PreambleTests.cpp:820
+  {
+// Note is also dropped if diag is gone.
+Annotations Code(R"(

i'm confused about what this comment is getting at - a note without a diag is 
not even representable


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143096/new/

https://reviews.llvm.org/D143096

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 496907.
kadircet edited the summary of this revision.
kadircet added a comment.

- Update commit message


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 [[]])");
 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 [[]])");
 Annotations NewCode("#include [[]]");
 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 [[]]");
 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 )");
 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 [[]]");
 Annotations NewCode(" # include ");
 auto AST = createPatchedAST(Code.code(), NewCode.code());
-// FIXME: No diags.
+EXPECT_THAT(*AST->getDiagnostics(), IsEmpty());
+  }
+  {
+// Multiple lines.
+Annotations Code(R"(
+#define BAR
+#include [[]])");
+Annotations NewCode(R"(#include [[]])");
+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 
+#include [[]])");
+Annotations NewCode(R"(#include )");
+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 
@@ -157,

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 496905.
kadircet added a comment.

- Reflow comments


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 [[]])");
 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 [[]])");
 Annotations NewCode("#include [[]]");
 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 [[]]");
 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 )");
 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 [[]]");
 Annotations NewCode(" # include ");
 auto AST = createPatchedAST(Code.code(), NewCode.code());
-// FIXME: No diags.
+EXPECT_THAT(*AST->getDiagnostics(), IsEmpty());
+  }
+  {
+// Multiple lines.
+Annotations Code(R"(
+#define BAR
+#include [[]])");
+Annotations NewCode(R"(#include [[]])");
+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 
+#include [[]])");
+Annotations NewCode(R"(#include )");
+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 
@@ -157,6 +158,8 @@
   /// Whether diagnostics generated usi

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

[Kadir Cetinkaya via Phabricator via cfe-commits]

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 [[]])");
 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 [[]])");
 Annotations NewCode("#include [[]]");
 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 [[]]");
 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 )");
 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 [[]]");
 Annotations NewCode(" # include ");
 auto AST = createPatchedAST(Code.code(), NewCode.code());
-// FIXME: No diags.
+EXPECT_THAT(*AST->getDiagnostics(), IsEmpty());
+  }
+  {
+// Multiple lines.
+Annotations Code(R"(
+#define BAR
+#include [[]])");
+Annotations NewCode(R"(#include [[]])");
+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 
+#include [[]])");
+Annotations NewCode(R"(#include )");
+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(), IsEmp

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/Preamble.cpp:648
+  });
+  if (NotePointsToOutside)
+return true;

Oh yeah, one more thing :-)

Currently if the note/fix aren't at this line we're bailing out of the 
diagnostic entirely.

We have three behaviors available:

1. patch the note/fix range as best we can and emit it
2. drop the note/fix but keep the diagnostic
3. drop the diagnostic entirely

I think we should generally prefer 1 when we can, maybe there's a case for 
choosing 2 sometimes (heuristics about when diagnostics have probably been 
invalidated, with comments), and we should ~never do 3.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143096/new/

https://reviews.llvm.org/D143096

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/ParsedAST.cpp:678
+if (Preamble) {
+  auto PDiags = Patch->patchedDiags();
+  Diags->insert(Diags->end(), PDiags.begin(), PDiags.end());

llvm::append_range(Diags, Patch->patchedDiags()) ?



Comment at: clang-tools-extra/clangd/Preamble.cpp:373
   SP.Includes = std::move(Includes.MainFileIncludes);
+  for (auto Pos = Contents.find('\n'); Pos != Contents.npos;
+   Contents = Contents.substr(Pos + 1), Pos = Contents.find('\n')) {

llvm::append_range(SP.Lines, llvm::split(Contents, "\n")) ?



Comment at: clang-tools-extra/clangd/Preamble.cpp:639
 
+// Checks if all pointers in \p D are for the same line of the main file.
+static bool diagReferencesMultipleLines(const Diag &D) {

nit: comment sense is the opposite of the function name

maybe say why this is an important property, or give the function a 
higher-level name?



Comment at: clang-tools-extra/clangd/Preamble.cpp:640
+// 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;

this function + translateDiag look like a validate/parse pair that need to be 
kept in sync.

they could be a single `translateDiag()` function that could fail, instead.



Comment at: clang-tools-extra/clangd/Preamble.cpp:645
+  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);

this check looks dubious to me:
 - `File` is a human-readable string with ill-defined format, comparing them 
for equality isn't safe, you want InsideMainFile instead
 - if the note points to another file, then it's not that translation is 
impossible, but rather no translation is needed
 - if the note points to the same file, then why can't we apply the same 
translation?

I think this should rather be something like:
```
bool translateDiag(DiagBase& D, bool IsMainDiag) {
  if (IsMainDiag) {
for (const auto& N : cast(D).Notes) {
  if (!translateDiag(N, /*IsMainDiag=*/false))
return false;
}
  }
  if (D.InsideMainFile) {
// attempt patching range of D itself
  }
}
```




Comment at: clang-tools-extra/clangd/Preamble.cpp:680
+static llvm::DenseMap>
+mapDiagsToLines(llvm::ArrayRef Diags) {
+  llvm::DenseMap> LineToDiags;

nit: name seems backwards (but as noted below I think this function can go away 
entirely)



Comment at: clang-tools-extra/clangd/Preamble.cpp:705
+// same spelling.
+static std::vector patchDiags(llvm::ArrayRef BaselineDiags,
+const ScannedPreamble &BaselineScan,

If I'm understanding the algorithm right:
 - we make a map of line content => original line number
 - we iterate through each line of the modified preamble, and:
- find the corresponding original line
- look for diagnostics to translate

Why not loop over diagnostics instead of lines in the modified preamble? There 
should be a lot fewer, and you avoid the lines-to-diags map.



Comment at: clang-tools-extra/clangd/Preamble.cpp:707
+const ScannedPreamble &BaselineScan,
+const ScannedPreamble &ModifiedScan) {
+  std::vector PatchedDiags;

large preamble with no diagnostics seems pretty plausible, maybe bail out early 
to avoid building the content map?



Comment at: clang-tools-extra/clangd/Preamble.cpp:795
 // Baseline for exclusion.
-llvm::DenseMap,
-   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);

I think this change and the IncludeKey class can be reverted now?



Comment at: clang-tools-extra/clangd/Preamble.h:161
 
+  /// Returns diag locations for Modified contents, only contains diags 
attached
+  /// to an #include or #define directive.

the stuff about `#include`/`#define` doesn't seem to correspond to anything in 
the code (obsolete?)
and below



Comment at: clang-tools-extra/clangd/Preamble.h:175
   std::string PatchFileName;
-  /// Includes that are present in both \p Baseline and \p Modified. Used for
-  /

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

[Kadir Cetinkaya via Phabricator via cfe-commits]

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 )");
 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 ");
 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 
@@ -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 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 PreambleIncludes;
+  // Diags that were attached to an #include or a #define directive.
+  std::vector 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 
+#include 
+#include 
+#include 
 #include 
 #include 
 #include 
@@ -306,6 +311,8 @@
 struct ScannedPreamble {
   std::vector Includes;
   std::vector TextualDirectives;
+

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked an inline comment as done.
kadircet added a comment.

In D143096#4099662 , @sammccall wrote:

> It looks like this fixes up the location only of diagnostics attached to 
> particular directives (`#include`) based on some "deep" idea about the 
> content of the directive (the spelled header name).
>
> Some shortcomings:
>
> - This misses diagnostics attached to other directives / continued lines in 
> macro definitions / etc
> - it allows diagnostics to be translated across lines even if the directive 
> spelling changed (in which case the ranges are incorrect since only line 
> numbers are updated.)
> - it requires modelling the directive content in some way that needs to be 
> extended if we find another place that diagnostics can be attached
>
> We discussed the idea of attaching to the text of the line, which seems 
> pretty generic. Needs some handling of duplicate line content but seems like 
> something pretty naive (closest line number with matching content?) would 
> work well, be just as simple and more generic. Any reason this alternative 
> was rejected?

agreed this is better, also somewhat more elegant on the implementation side. 
switching to a line-content based translation, while keeping the logic around 
limiting diagnostics to single lines. as multiple lines seem more wonky and 
unclear how common they're.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143096/new/

https://reviews.llvm.org/D143096

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

It looks like this fixes up the location only of diagnostics attached to 
particular directives (`#include`) based on some "deep" idea about the content 
of the directive (the spelled header name).

Some shortcomings:

- This misses diagnostics attached to other directives / continued lines in 
macro definitions / etc
- it allows diagnostics to be translated across lines even if the directive 
spelling changed (in which case the ranges are incorrect since only line 
numbers are updated.)
- it requires modelling the directive content in some way that needs to be 
extended if we find another place that diagnostics can be attached

We discussed the idea of attaching to the text of the line, which seems pretty 
generic. Needs some handling of duplicate line content but seems like something 
pretty naive (closest line number with matching content?) would work well, be 
just as simple and more generic. Any reason this alternative was rejected?




Comment at: clang-tools-extra/clangd/Preamble.h:162
+  /// to an #include or #define directive.
+  std::vector patchedDiags() const { return PatchedDiags; }
   static constexpr llvm::StringLiteral HeaderName = "__preamble_patch__.h";

this looks like a gratuitous copy - ParsedAST copies again


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143096/new/

https://reviews.llvm.org/D143096

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

[Kadir Cetinkaya via Phabricator via cfe-commits]

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 


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 [[]]\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 [[]]\n#include \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