subject:"\[PATCH\] D59302\: \[clangd\] Surface diagnostics from headers inside main file"

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-04-29 Thread Kadir Cetinkaya via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rL359432: [clangd] Surface diagnostics from headers inside 
main file (authored by kadircet, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D59302

Files:
  clang-tools-extra/trunk/clangd/Diagnostics.cpp
  clang-tools-extra/trunk/clangd/Diagnostics.h
  clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp
  clang-tools-extra/trunk/clangd/unittests/TestTU.cpp
  clang-tools-extra/trunk/clangd/unittests/TestTU.h

Index: clang-tools-extra/trunk/clangd/Diagnostics.cpp
===
--- clang-tools-extra/trunk/clangd/Diagnostics.cpp
+++ clang-tools-extra/trunk/clangd/Diagnostics.cpp
@@ -13,11 +13,18 @@
 #include "Protocol.h"
 #include "SourceCode.h"
 #include "clang/Basic/AllDiagnostics.h"
+#include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticIDs.h"
+#include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/Twine.h"
 #include "llvm/Support/Capacity.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/ScopedPrinter.h"
+#include "llvm/Support/Signals.h"
 #include 
 
 namespace clang {
@@ -102,6 +109,39 @@
   return halfOpenToRange(M, R);
 }
 
+void adjustDiagFromHeader(Diag &D, const clang::Diagnostic &Info,
+  const LangOptions &LangOpts) {
+  const SourceLocation &DiagLoc = Info.getLocation();
+  const SourceManager &SM = Info.getSourceManager();
+  SourceLocation IncludeInMainFile;
+  auto GetIncludeLoc = [&SM](SourceLocation SLoc) {
+return SM.getIncludeLoc(SM.getFileID(SLoc));
+  };
+  for (auto IncludeLocation = GetIncludeLoc(DiagLoc); IncludeLocation.isValid();
+   IncludeLocation = GetIncludeLoc(IncludeLocation))
+IncludeInMainFile = IncludeLocation;
+  if (IncludeInMainFile.isInvalid())
+return;
+
+  // Update diag to point at include inside main file.
+  D.File = SM.getFileEntryForID(SM.getMainFileID())->getName().str();
+  D.Range.start = sourceLocToPosition(SM, IncludeInMainFile);
+  D.Range.end = sourceLocToPosition(
+  SM, Lexer::getLocForEndOfToken(IncludeInMainFile, 0, SM, LangOpts));
+
+  // Add a note that will point to real diagnostic.
+  const auto *FE = SM.getFileEntryForID(SM.getFileID(DiagLoc));
+  D.Notes.emplace_back();
+  Note &N = D.Notes.back();
+  N.AbsFile = FE->tryGetRealPathName();
+  N.File = FE->getName();
+  N.Message = "error occurred here";
+  N.Range = diagnosticRange(Info, LangOpts);
+
+  // Update message to mention original file.
+  D.Message = llvm::Twine("in included file: ", D.Message).str();
+}
+
 bool isInsideMainFile(const SourceLocation Loc, const SourceManager &M) {
   return Loc.isValid() && M.isWrittenInMainFile(M.getFileLoc(Loc));
 }
@@ -378,7 +418,7 @@
 Msg.resize(Rest.size());
 };
 CleanMessage(Diag.Message);
-for (auto& Note : Diag.Notes)
+for (auto &Note : Diag.Notes)
   CleanMessage(Note.Message);
 continue;
   }
@@ -477,6 +517,7 @@
 LastDiag = Diag();
 LastDiag->ID = Info.getID();
 FillDiagBase(*LastDiag);
+adjustDiagFromHeader(*LastDiag, Info, *LangOpts);
 
 if (!Info.getFixItHints().empty())
   AddFix(true /* try to invent a message instead of repeating the diag */);
@@ -511,11 +552,15 @@
 void StoreDiags::flushLastDiag() {
   if (!LastDiag)
 return;
-  if (mentionsMainFile(*LastDiag))
+  // Only keeps diagnostics inside main file or the first one coming from a
+  // header.
+  if (mentionsMainFile(*LastDiag) ||
+  (LastDiag->Severity >= DiagnosticsEngine::Level::Error &&
+   IncludeLinesWithErrors.insert(LastDiag->Range.start.line).second)) {
 Output.push_back(std::move(*LastDiag));
-  else
-vlog("Dropped diagnostic outside main file: {0}: {1}", LastDiag->File,
- LastDiag->Message);
+  } else {
+vlog("Dropped diagnostic: {0}: {1}", LastDiag->File, LastDiag->Message);
+  }
   LastDiag.reset();
 }
 
Index: clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp
@@ -9,10 +9,11 @@
 #include "Annotations.h"
 #include "ClangdUnit.h"
 #include "Diagnostics.h"
+#include "Path.h"
 #include "Protocol.h"
 #include "SourceCode.h"
-#include "TestIndex.h"
 #include "TestFS.h"
+#include "TestIndex.h"
 #include "TestTU.h"
 #include "index/MemIndex.h"
 #include "clang/Basic/Diagnostic.h"
@@ -20,6 +21,7 @@
 #include "llvm/Support/ScopedPrinter.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include 
 
 namespace clang {
 namespace clangd {
@@ -6

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-04-29 Thread Kadir Cetinkaya via Phabricator via cfe-commits

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

- Rebase


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302

Files:
  clangd/Diagnostics.cpp
  clangd/Diagnostics.h
  clangd/unittests/DiagnosticsTests.cpp
  clangd/unittests/TestTU.cpp
  clangd/unittests/TestTU.h

Index: clangd/unittests/TestTU.h
===
--- clangd/unittests/TestTU.h
+++ clangd/unittests/TestTU.h
@@ -18,8 +18,13 @@
 #define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTTU_H
 
 #include "ClangdUnit.h"
+#include "Path.h"
 #include "index/Index.h"
+#include "llvm/ADT/StringMap.h"
 #include "gtest/gtest.h"
+#include 
+#include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -45,6 +50,9 @@
   std::string HeaderCode;
   std::string HeaderFilename = "TestTU.h";
 
+  // Name and contents of each file.
+  llvm::StringMap AdditionalFiles;
+
   // Extra arguments for the compiler invocation.
   std::vector ExtraArgs;
 
Index: clangd/unittests/TestTU.cpp
===
--- clangd/unittests/TestTU.cpp
+++ clangd/unittests/TestTU.cpp
@@ -21,11 +21,17 @@
   std::string FullFilename = testPath(Filename),
   FullHeaderName = testPath(HeaderFilename),
   ImportThunk = testPath("import_thunk.h");
-  std::vector Cmd = {"clang", FullFilename.c_str()};
   // We want to implicitly include HeaderFilename without messing up offsets.
   // -include achieves this, but sometimes we want #import (to simulate a header
   // guard without messing up offsets). In this case, use an intermediate file.
   std::string ThunkContents = "#import \"" + FullHeaderName + "\"\n";
+
+  llvm::StringMap Files(AdditionalFiles);
+  Files[FullFilename] = Code;
+  Files[FullHeaderName] = HeaderCode;
+  Files[ImportThunk] = ThunkContents;
+
+  std::vector Cmd = {"clang", FullFilename.c_str()};
   // FIXME: this shouldn't need to be conditional, but it breaks a
   // GoToDefinition test for some reason (getMacroArgExpandedLocation fails).
   if (!HeaderCode.empty()) {
@@ -39,9 +45,7 @@
   Inputs.CompileCommand.CommandLine = {Cmd.begin(), Cmd.end()};
   Inputs.CompileCommand.Directory = testRoot();
   Inputs.Contents = Code;
-  Inputs.FS = buildTestFS({{FullFilename, Code},
-   {FullHeaderName, HeaderCode},
-   {ImportThunk, ThunkContents}});
+  Inputs.FS = buildTestFS(Files);
   Inputs.Opts = ParseOptions();
   Inputs.Opts.ClangTidyOpts.Checks = ClangTidyChecks;
   Inputs.Index = ExternalIndex;
Index: clangd/unittests/DiagnosticsTests.cpp
===
--- clangd/unittests/DiagnosticsTests.cpp
+++ clangd/unittests/DiagnosticsTests.cpp
@@ -9,10 +9,11 @@
 #include "Annotations.h"
 #include "ClangdUnit.h"
 #include "Diagnostics.h"
+#include "Path.h"
 #include "Protocol.h"
 #include "SourceCode.h"
-#include "TestIndex.h"
 #include "TestFS.h"
+#include "TestIndex.h"
 #include "TestTU.h"
 #include "index/MemIndex.h"
 #include "clang/Basic/Diagnostic.h"
@@ -20,6 +21,7 @@
 #include "llvm/Support/ScopedPrinter.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include 
 
 namespace clang {
 namespace clangd {
@@ -61,7 +63,8 @@
   "LSP diagnostic " + llvm::to_string(LSPDiag)) {
   if (toJSON(arg) != toJSON(LSPDiag)) {
 *result_listener << llvm::formatv("expected:\n{0:2}\ngot\n{1:2}",
-  toJSON(LSPDiag), toJSON(arg)).str();
+  toJSON(LSPDiag), toJSON(arg))
+.str();
 return false;
   }
   return true;
@@ -83,7 +86,6 @@
   return true;
 }
 
-
 // Helper function to make tests shorter.
 Position pos(int line, int character) {
   Position Res;
@@ -114,8 +116,7 @@
   // This range spans lines.
   AllOf(Diag(Test.range("typo"),
  "use of undeclared identifier 'goo'; did you mean 'foo'?"),
-DiagSource(Diag::Clang),
-DiagName("undeclared_var_use_suggest"),
+DiagSource(Diag::Clang), DiagName("undeclared_var_use_suggest"),
 WithFix(
 Fix(Test.range("typo"), "foo", "change 'go\\ o' to 'foo'")),
 // This is a pretty normal range.
@@ -301,7 +302,8 @@
   MainLSP.severity = getSeverity(DiagnosticsEngine::Error);
   MainLSP.code = "enum_class_reference";
   MainLSP.source = "clang";
-  MainLSP.message = R"(Something terrible happened (fix available)
+  MainLSP.message =
+  R"(Something terrible happened (fix available)
 
 main.cpp:6:7: remark: declared somewhere in the main file
 
@@ -354,9 +356,8 @@
   NoteInHeaderDRI.location.range = NoteInHeader.Range;
   NoteInHeaderDRI.location.uri = HeaderFile;
   MainLSP.relatedInformation = {NoteInMainDRI, NoteInHeaderDRI};
-  EXPECT_THAT(
-  LSPDiags,
-  Elemen

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-04-29 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet updated this revision to Diff 197073.
kadircet marked 8 inline comments as done.
kadircet added a comment.

- Address comments


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302

Files:
  clangd/Diagnostics.cpp
  clangd/Diagnostics.h
  unittests/clangd/DiagnosticsTests.cpp
  unittests/clangd/TestTU.cpp
  unittests/clangd/TestTU.h

Index: unittests/clangd/TestTU.h
===
--- unittests/clangd/TestTU.h
+++ unittests/clangd/TestTU.h
@@ -18,8 +18,13 @@
 #define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTTU_H
 
 #include "ClangdUnit.h"
+#include "Path.h"
 #include "index/Index.h"
+#include "llvm/ADT/StringMap.h"
 #include "gtest/gtest.h"
+#include 
+#include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -45,6 +50,9 @@
   std::string HeaderCode;
   std::string HeaderFilename = "TestTU.h";
 
+  // Name and contents of each file.
+  llvm::StringMap AdditionalFiles;
+
   // Extra arguments for the compiler invocation.
   std::vector ExtraArgs;
 
Index: unittests/clangd/TestTU.cpp
===
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -21,11 +21,17 @@
   std::string FullFilename = testPath(Filename),
   FullHeaderName = testPath(HeaderFilename),
   ImportThunk = testPath("import_thunk.h");
-  std::vector Cmd = {"clang", FullFilename.c_str()};
   // We want to implicitly include HeaderFilename without messing up offsets.
   // -include achieves this, but sometimes we want #import (to simulate a header
   // guard without messing up offsets). In this case, use an intermediate file.
   std::string ThunkContents = "#import \"" + FullHeaderName + "\"\n";
+
+  llvm::StringMap Files(AdditionalFiles);
+  Files[FullFilename] = Code;
+  Files[FullHeaderName] = HeaderCode;
+  Files[ImportThunk] = ThunkContents;
+
+  std::vector Cmd = {"clang", FullFilename.c_str()};
   // FIXME: this shouldn't need to be conditional, but it breaks a
   // GoToDefinition test for some reason (getMacroArgExpandedLocation fails).
   if (!HeaderCode.empty()) {
@@ -39,9 +45,7 @@
   Inputs.CompileCommand.CommandLine = {Cmd.begin(), Cmd.end()};
   Inputs.CompileCommand.Directory = testRoot();
   Inputs.Contents = Code;
-  Inputs.FS = buildTestFS({{FullFilename, Code},
-   {FullHeaderName, HeaderCode},
-   {ImportThunk, ThunkContents}});
+  Inputs.FS = buildTestFS(Files);
   Inputs.Opts = ParseOptions();
   Inputs.Opts.ClangTidyOpts.Checks = ClangTidyChecks;
   Inputs.Index = ExternalIndex;
Index: unittests/clangd/DiagnosticsTests.cpp
===
--- unittests/clangd/DiagnosticsTests.cpp
+++ unittests/clangd/DiagnosticsTests.cpp
@@ -9,10 +9,11 @@
 #include "Annotations.h"
 #include "ClangdUnit.h"
 #include "Diagnostics.h"
+#include "Path.h"
 #include "Protocol.h"
 #include "SourceCode.h"
-#include "TestIndex.h"
 #include "TestFS.h"
+#include "TestIndex.h"
 #include "TestTU.h"
 #include "index/MemIndex.h"
 #include "clang/Basic/Diagnostic.h"
@@ -20,6 +21,7 @@
 #include "llvm/Support/ScopedPrinter.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include 
 
 namespace clang {
 namespace clangd {
@@ -61,7 +63,8 @@
   "LSP diagnostic " + llvm::to_string(LSPDiag)) {
   if (toJSON(arg) != toJSON(LSPDiag)) {
 *result_listener << llvm::formatv("expected:\n{0:2}\ngot\n{1:2}",
-  toJSON(LSPDiag), toJSON(arg)).str();
+  toJSON(LSPDiag), toJSON(arg))
+.str();
 return false;
   }
   return true;
@@ -83,7 +86,6 @@
   return true;
 }
 
-
 // Helper function to make tests shorter.
 Position pos(int line, int character) {
   Position Res;
@@ -114,8 +116,7 @@
   // This range spans lines.
   AllOf(Diag(Test.range("typo"),
  "use of undeclared identifier 'goo'; did you mean 'foo'?"),
-DiagSource(Diag::Clang),
-DiagName("undeclared_var_use_suggest"),
+DiagSource(Diag::Clang), DiagName("undeclared_var_use_suggest"),
 WithFix(
 Fix(Test.range("typo"), "foo", "change 'go\\ o' to 'foo'")),
 // This is a pretty normal range.
@@ -301,7 +302,8 @@
   MainLSP.severity = getSeverity(DiagnosticsEngine::Error);
   MainLSP.code = "enum_class_reference";
   MainLSP.source = "clang";
-  MainLSP.message = R"(Something terrible happened (fix available)
+  MainLSP.message =
+  R"(Something terrible happened (fix available)
 
 main.cpp:6:7: remark: declared somewhere in the main file
 
@@ -354,9 +356,8 @@
   NoteInHeaderDRI.location.range = NoteInHeader.Range;
   NoteInHeaderDRI.location.uri = HeaderFile;
   MainLSP.relatedInformation = {NoteInMainDRI, NoteInHeade

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-04-29 Thread Sam McCall via Phabricator via cfe-commits

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

Nice! Just readability/wording nits




Comment at: clangd/Diagnostics.cpp:463
+  // header.
+  auto ShouldAddDiag = [this](const Diag &D) {
+if (mentionsMainFile(D))

ilya-biryukov wrote:
> Could you inline `ShouldAddDiag` into its single callsite?
Does this improve readability over (inline) `D.Severity >= 
DiagnosticsEngine::Error`?



Comment at: clangd/Diagnostics.cpp:137
+  N.File = FE->getName();
+  N.Message = "Error origin";
+  N.Range = diagnosticRange(Info, LangOpts);

Clang tends to use a phrase ending in "here" for such notes.
Suggest "error occurred here"



Comment at: clangd/Diagnostics.cpp:137
+  N.File = FE->getName();
+  N.Message = "Error origin";
+  N.Range = diagnosticRange(Info, LangOpts);

sammccall wrote:
> Clang tends to use a phrase ending in "here" for such notes.
> Suggest "error occurred here"
message should not be capitalized (capitalization will be added later if needed)



Comment at: clangd/Diagnostics.cpp:141
+  // Update message to mention original file.
+  D.Message = (llvm::Twine("Error in include ",
+   llvm::sys::path::filename(FE->getName())) +

Include is not a noun. "included file"?



Comment at: clangd/Diagnostics.cpp:141
+  // Update message to mention original file.
+  D.Message = (llvm::Twine("Error in include ",
+   llvm::sys::path::filename(FE->getName())) +

sammccall wrote:
> Include is not a noun. "included file"?
"Error" is the severity, which appears elsewhere in LSP. (Other messages do not 
include it)



Comment at: clangd/Diagnostics.cpp:141
+  // Update message to mention original file.
+  D.Message = (llvm::Twine("Error in include ",
+   llvm::sys::path::filename(FE->getName())) +

sammccall wrote:
> sammccall wrote:
> > Include is not a noun. "included file"?
> "Error" is the severity, which appears elsewhere in LSP. (Other messages do 
> not include it)
message should not be capitalized



Comment at: clangd/Diagnostics.cpp:142
+  D.Message = (llvm::Twine("Error in include ",
+   llvm::sys::path::filename(FE->getName())) +
+   ": " + D.Message)

why are we including the filename here?

it obscures the error message, and will be available via the note.



Comment at: clangd/Diagnostics.h:141
   llvm::Optional LastDiag;
+  /// Stores lines of the includes containing a diagnostic.
+  llvm::DenseSet IncludeHasDiag;

nit: "containing an error" (and variable name)

IncludeLinesWithErrors might be a clearer name


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302



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

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-04-29 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet updated this revision to Diff 197068.
kadircet marked 2 inline comments as done.
kadircet added a comment.

- Address comments


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302

Files:
  clangd/Diagnostics.cpp
  clangd/Diagnostics.h
  unittests/clangd/DiagnosticsTests.cpp
  unittests/clangd/TestTU.cpp
  unittests/clangd/TestTU.h

Index: unittests/clangd/TestTU.h
===
--- unittests/clangd/TestTU.h
+++ unittests/clangd/TestTU.h
@@ -18,8 +18,13 @@
 #define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTTU_H
 
 #include "ClangdUnit.h"
+#include "Path.h"
 #include "index/Index.h"
+#include "llvm/ADT/StringMap.h"
 #include "gtest/gtest.h"
+#include 
+#include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -45,6 +50,9 @@
   std::string HeaderCode;
   std::string HeaderFilename = "TestTU.h";
 
+  // Name and contents of each file.
+  llvm::StringMap AdditionalFiles;
+
   // Extra arguments for the compiler invocation.
   std::vector ExtraArgs;
 
Index: unittests/clangd/TestTU.cpp
===
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -21,11 +21,17 @@
   std::string FullFilename = testPath(Filename),
   FullHeaderName = testPath(HeaderFilename),
   ImportThunk = testPath("import_thunk.h");
-  std::vector Cmd = {"clang", FullFilename.c_str()};
   // We want to implicitly include HeaderFilename without messing up offsets.
   // -include achieves this, but sometimes we want #import (to simulate a header
   // guard without messing up offsets). In this case, use an intermediate file.
   std::string ThunkContents = "#import \"" + FullHeaderName + "\"\n";
+
+  llvm::StringMap Files(AdditionalFiles);
+  Files[FullFilename] = Code;
+  Files[FullHeaderName] = HeaderCode;
+  Files[ImportThunk] = ThunkContents;
+
+  std::vector Cmd = {"clang", FullFilename.c_str()};
   // FIXME: this shouldn't need to be conditional, but it breaks a
   // GoToDefinition test for some reason (getMacroArgExpandedLocation fails).
   if (!HeaderCode.empty()) {
@@ -39,9 +45,7 @@
   Inputs.CompileCommand.CommandLine = {Cmd.begin(), Cmd.end()};
   Inputs.CompileCommand.Directory = testRoot();
   Inputs.Contents = Code;
-  Inputs.FS = buildTestFS({{FullFilename, Code},
-   {FullHeaderName, HeaderCode},
-   {ImportThunk, ThunkContents}});
+  Inputs.FS = buildTestFS(Files);
   Inputs.Opts = ParseOptions();
   Inputs.Opts.ClangTidyOpts.Checks = ClangTidyChecks;
   Inputs.Index = ExternalIndex;
Index: unittests/clangd/DiagnosticsTests.cpp
===
--- unittests/clangd/DiagnosticsTests.cpp
+++ unittests/clangd/DiagnosticsTests.cpp
@@ -9,10 +9,11 @@
 #include "Annotations.h"
 #include "ClangdUnit.h"
 #include "Diagnostics.h"
+#include "Path.h"
 #include "Protocol.h"
 #include "SourceCode.h"
-#include "TestIndex.h"
 #include "TestFS.h"
+#include "TestIndex.h"
 #include "TestTU.h"
 #include "index/MemIndex.h"
 #include "clang/Basic/Diagnostic.h"
@@ -20,6 +21,7 @@
 #include "llvm/Support/ScopedPrinter.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include 
 
 namespace clang {
 namespace clangd {
@@ -61,7 +63,8 @@
   "LSP diagnostic " + llvm::to_string(LSPDiag)) {
   if (toJSON(arg) != toJSON(LSPDiag)) {
 *result_listener << llvm::formatv("expected:\n{0:2}\ngot\n{1:2}",
-  toJSON(LSPDiag), toJSON(arg)).str();
+  toJSON(LSPDiag), toJSON(arg))
+.str();
 return false;
   }
   return true;
@@ -83,7 +86,6 @@
   return true;
 }
 
-
 // Helper function to make tests shorter.
 Position pos(int line, int character) {
   Position Res;
@@ -114,8 +116,7 @@
   // This range spans lines.
   AllOf(Diag(Test.range("typo"),
  "use of undeclared identifier 'goo'; did you mean 'foo'?"),
-DiagSource(Diag::Clang),
-DiagName("undeclared_var_use_suggest"),
+DiagSource(Diag::Clang), DiagName("undeclared_var_use_suggest"),
 WithFix(
 Fix(Test.range("typo"), "foo", "change 'go\\ o' to 'foo'")),
 // This is a pretty normal range.
@@ -301,7 +302,8 @@
   MainLSP.severity = getSeverity(DiagnosticsEngine::Error);
   MainLSP.code = "enum_class_reference";
   MainLSP.source = "clang";
-  MainLSP.message = R"(Something terrible happened (fix available)
+  MainLSP.message =
+  R"(Something terrible happened (fix available)
 
 main.cpp:6:7: remark: declared somewhere in the main file
 
@@ -354,9 +356,8 @@
   NoteInHeaderDRI.location.range = NoteInHeader.Range;
   NoteInHeaderDRI.location.uri = HeaderFile;
   MainLSP.relatedInformation = {NoteInMainDRI, NoteInHeade

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-04-29 Thread Ilya Biryukov via Phabricator via cfe-commits

ilya-biryukov added inline comments.



Comment at: clangd/Diagnostics.cpp:115
+  const SourceManager &SM = Info.getSourceManager();
+  std::vector IncludeStack;
+  auto GetIncludeLoc = [&SM](SourceLocation SLoc) {

replace `vector` with `SourceLocation` now that we only need 
one of the locations.



Comment at: unittests/clangd/DiagnosticsTests.cpp:748
+  UnorderedElementsAre(
+  Diag(Main.range(), "Error in include c.h: C++ requires a "
+ "type specifier for all declarations")));

NIT: maybe quote the name of the header for better readability: `error in 
include 'c.h' ...`?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302



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

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-04-29 Thread Ilya Biryukov via Phabricator via cfe-commits

ilya-biryukov added a comment.

This looks nice and minimal, thanks!
Happy to LGTM this, unless @sammccall want to take another look (e.g. to 
account for related information patch)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302



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

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-04-26 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet updated this revision to Diff 196822.
kadircet marked 5 inline comments as done.
kadircet added a comment.

- Address comments
- Drop include stack


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302

Files:
  clangd/Diagnostics.cpp
  clangd/Diagnostics.h
  unittests/clangd/DiagnosticsTests.cpp
  unittests/clangd/TestTU.cpp
  unittests/clangd/TestTU.h

Index: unittests/clangd/TestTU.h
===
--- unittests/clangd/TestTU.h
+++ unittests/clangd/TestTU.h
@@ -18,8 +18,13 @@
 #define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTTU_H
 
 #include "ClangdUnit.h"
+#include "Path.h"
 #include "index/Index.h"
+#include "llvm/ADT/StringMap.h"
 #include "gtest/gtest.h"
+#include 
+#include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -45,6 +50,9 @@
   std::string HeaderCode;
   std::string HeaderFilename = "TestTU.h";
 
+  // Name and contents of each file.
+  llvm::StringMap AdditionalFiles;
+
   // Extra arguments for the compiler invocation.
   std::vector ExtraArgs;
 
Index: unittests/clangd/TestTU.cpp
===
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -21,11 +21,17 @@
   std::string FullFilename = testPath(Filename),
   FullHeaderName = testPath(HeaderFilename),
   ImportThunk = testPath("import_thunk.h");
-  std::vector Cmd = {"clang", FullFilename.c_str()};
   // We want to implicitly include HeaderFilename without messing up offsets.
   // -include achieves this, but sometimes we want #import (to simulate a header
   // guard without messing up offsets). In this case, use an intermediate file.
   std::string ThunkContents = "#import \"" + FullHeaderName + "\"\n";
+
+  llvm::StringMap Files(AdditionalFiles);
+  Files[FullFilename] = Code;
+  Files[FullHeaderName] = HeaderCode;
+  Files[ImportThunk] = ThunkContents;
+
+  std::vector Cmd = {"clang", FullFilename.c_str()};
   // FIXME: this shouldn't need to be conditional, but it breaks a
   // GoToDefinition test for some reason (getMacroArgExpandedLocation fails).
   if (!HeaderCode.empty()) {
@@ -39,9 +45,7 @@
   Inputs.CompileCommand.CommandLine = {Cmd.begin(), Cmd.end()};
   Inputs.CompileCommand.Directory = testRoot();
   Inputs.Contents = Code;
-  Inputs.FS = buildTestFS({{FullFilename, Code},
-   {FullHeaderName, HeaderCode},
-   {ImportThunk, ThunkContents}});
+  Inputs.FS = buildTestFS(Files);
   Inputs.Opts = ParseOptions();
   Inputs.Opts.ClangTidyOpts.Checks = ClangTidyChecks;
   Inputs.Index = ExternalIndex;
Index: unittests/clangd/DiagnosticsTests.cpp
===
--- unittests/clangd/DiagnosticsTests.cpp
+++ unittests/clangd/DiagnosticsTests.cpp
@@ -9,10 +9,11 @@
 #include "Annotations.h"
 #include "ClangdUnit.h"
 #include "Diagnostics.h"
+#include "Path.h"
 #include "Protocol.h"
 #include "SourceCode.h"
-#include "TestIndex.h"
 #include "TestFS.h"
+#include "TestIndex.h"
 #include "TestTU.h"
 #include "index/MemIndex.h"
 #include "clang/Basic/Diagnostic.h"
@@ -20,6 +21,7 @@
 #include "llvm/Support/ScopedPrinter.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include 
 
 namespace clang {
 namespace clangd {
@@ -61,7 +63,8 @@
   "LSP diagnostic " + llvm::to_string(LSPDiag)) {
   if (toJSON(arg) != toJSON(LSPDiag)) {
 *result_listener << llvm::formatv("expected:\n{0:2}\ngot\n{1:2}",
-  toJSON(LSPDiag), toJSON(arg)).str();
+  toJSON(LSPDiag), toJSON(arg))
+.str();
 return false;
   }
   return true;
@@ -83,7 +86,6 @@
   return true;
 }
 
-
 // Helper function to make tests shorter.
 Position pos(int line, int character) {
   Position Res;
@@ -114,8 +116,7 @@
   // This range spans lines.
   AllOf(Diag(Test.range("typo"),
  "use of undeclared identifier 'goo'; did you mean 'foo'?"),
-DiagSource(Diag::Clang),
-DiagName("undeclared_var_use_suggest"),
+DiagSource(Diag::Clang), DiagName("undeclared_var_use_suggest"),
 WithFix(
 Fix(Test.range("typo"), "foo", "change 'go\\ o' to 'foo'")),
 // This is a pretty normal range.
@@ -301,7 +302,8 @@
   MainLSP.severity = getSeverity(DiagnosticsEngine::Error);
   MainLSP.code = "enum_class_reference";
   MainLSP.source = "clang";
-  MainLSP.message = R"(Something terrible happened (fix available)
+  MainLSP.message =
+  R"(Something terrible happened (fix available)
 
 main.cpp:6:7: remark: declared somewhere in the main file
 
@@ -354,9 +356,8 @@
   NoteInHeaderDRI.location.range = NoteInHeader.Range;
   NoteInHeaderDRI.location.uri = HeaderFile;
   MainLSP.relatedInformation = {NoteI

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-04-26 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet added inline comments.



Comment at: clangd/Diagnostics.cpp:78
+// offsets when displaying that information to users.
+Position toOneBased(Position P) {
+  ++P.line;

ilya-biryukov wrote:
> Could we avoid introducing a function that breaks the invariant of a type?
> Having a function to print `Position` differently would be a much better fit.
No longer needed reverting.



Comment at: clangd/Diagnostics.cpp:225
+const auto &Inc = D.IncludeStack[I];
+OS << "\nIn file included from: " << Inc.first << ':'
+   << toOneBased(Inc.second);

ilya-biryukov wrote:
> WDYT about changing this to a shorter form?
> ```
> include stack:
> 
> ./project/foo.h:312
> /usr/include/vector:344
> ```
> Note that it does not mention column numbers as they aren't useful and is 
> shorter.
> Since we're already not 100% aligned with clang, why not have a more concise 
> representation?
As discussed offline getting rid of include stack in this version of the patch.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302



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

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-04-25 Thread Ilya Biryukov via Phabricator via cfe-commits

ilya-biryukov added inline comments.



Comment at: clangd/Diagnostics.cpp:78
+// offsets when displaying that information to users.
+Position toOneBased(Position P) {
+  ++P.line;

Could we avoid introducing a function that breaks the invariant of a type?
Having a function to print `Position` differently would be a much better fit.



Comment at: clangd/Diagnostics.cpp:225
+const auto &Inc = D.IncludeStack[I];
+OS << "\nIn file included from: " << Inc.first << ':'
+   << toOneBased(Inc.second);

WDYT about changing this to a shorter form?
```
include stack:

./project/foo.h:312
/usr/include/vector:344
```
Note that it does not mention column numbers as they aren't useful and is 
shorter.
Since we're already not 100% aligned with clang, why not have a more concise 
representation?



Comment at: unittests/clangd/DiagnosticsTests.cpp:739
+   {"/clangd-test/a.h", pos(0, 9)},
+   {"/clangd-test/c.h", pos(3, 6)}})));
+}

So the last location in the include stack is actually the original location of 
an error.
This is non-obvious design. Maybe move this into a separate field?



Comment at: unittests/clangd/TestTU.h:51
 
+  // Absolute path and contents of each file.
+  std::vector> AdditionalFiles;

Maybe use relative paths?
This would be consistent with `Filename` and `HeaderFilename`



Comment at: unittests/clangd/TestTU.h:52
+  // Absolute path and contents of each file.
+  std::vector> AdditionalFiles;
+

Why not `StringMap`?
This would be consistent with `buildTestFS` and disallow adding duplicates.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302



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

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-04-16 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet added inline comments.



Comment at: unittests/clangd/DiagnosticsTests.cpp:620
 
+ParsedAST build(const std::string &MainFile,
+const llvm::StringMap &Files) {

ilya-biryukov wrote:
> We were discussing adding the extra files to `TestTU` instead.
> Any reason this did not work out?
Yes, my LRU cache evicted the discussion :D


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302



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

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-04-16 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet updated this revision to Diff 195375.
kadircet marked 14 inline comments as done.
kadircet added a comment.

- Address comments


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302

Files:
  clangd/Diagnostics.cpp
  clangd/Diagnostics.h
  unittests/clangd/DiagnosticsTests.cpp
  unittests/clangd/TestTU.cpp
  unittests/clangd/TestTU.h

Index: unittests/clangd/TestTU.h
===
--- unittests/clangd/TestTU.h
+++ unittests/clangd/TestTU.h
@@ -18,8 +18,11 @@
 #define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTTU_H
 
 #include "ClangdUnit.h"
+#include "Path.h"
 #include "index/Index.h"
 #include "gtest/gtest.h"
+#include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -45,6 +48,9 @@
   std::string HeaderCode;
   std::string HeaderFilename = "TestTU.h";
 
+  // Absolute path and contents of each file.
+  std::vector> AdditionalFiles;
+
   // Extra arguments for the compiler invocation.
   std::vector ExtraArgs;
 
Index: unittests/clangd/TestTU.cpp
===
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -20,6 +20,13 @@
 ParsedAST TestTU::build() const {
   std::string FullFilename = testPath(Filename),
   FullHeaderName = testPath(HeaderFilename);
+
+  llvm::StringMap Files;
+  Files[FullFilename] = Code;
+  Files[FullHeaderName] = HeaderCode;
+  for (const auto &Entry : AdditionalFiles)
+Files[Entry.first] = Entry.second;
+
   std::vector Cmd = {"clang", FullFilename.c_str()};
   // FIXME: this shouldn't need to be conditional, but it breaks a
   // GoToDefinition test for some reason (getMacroArgExpandedLocation fails).
@@ -33,7 +40,7 @@
   Inputs.CompileCommand.CommandLine = {Cmd.begin(), Cmd.end()};
   Inputs.CompileCommand.Directory = testRoot();
   Inputs.Contents = Code;
-  Inputs.FS = buildTestFS({{FullFilename, Code}, {FullHeaderName, HeaderCode}});
+  Inputs.FS = buildTestFS(Files);
   Inputs.Opts = ParseOptions();
   Inputs.Opts.ClangTidyOpts.Checks = ClangTidyChecks;
   Inputs.Index = ExternalIndex;
Index: unittests/clangd/DiagnosticsTests.cpp
===
--- unittests/clangd/DiagnosticsTests.cpp
+++ unittests/clangd/DiagnosticsTests.cpp
@@ -8,13 +8,17 @@
 
 #include "Annotations.h"
 #include "ClangdUnit.h"
+#include "Path.h"
+#include "Protocol.h"
 #include "SourceCode.h"
+#include "TestFS.h"
 #include "TestIndex.h"
 #include "TestTU.h"
 #include "index/MemIndex.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include 
 
 namespace clang {
 namespace clangd {
@@ -45,6 +49,15 @@
   return arg.Range == Range && arg.Message == Message;
 }
 
+MATCHER_P3(Diag, Range, Message, IncludeStack,
+   "Diag at " + llvm::to_string(Range) + " = [" + Message + "]") {
+  if (arg.Range != Range || arg.Message != Message ||
+  arg.IncludeStack.size() != IncludeStack.size())
+return false;
+  return std::equal(IncludeStack.begin(), IncludeStack.end(),
+arg.IncludeStack.begin());
+}
+
 MATCHER_P3(Fix, Range, Replacement, Message,
"Fix " + llvm::to_string(Range) + " => " +
testing::PrintToString(Replacement) + " = [" + Message + "]") {
@@ -73,7 +86,6 @@
   return true;
 }
 
-
 // Helper function to make tests shorter.
 Position pos(int line, int character) {
   Position Res;
@@ -251,6 +263,8 @@
   D.InsideMainFile = true;
   D.Severity = DiagnosticsEngine::Error;
   D.File = "foo/bar/main.cpp";
+  D.IncludeStack.push_back({"a/b.h", pos(0, 1)});
+  D.IncludeStack.push_back({"a/c.h", pos(1, 1)});
 
   clangd::Note NoteInMain;
   NoteInMain.Message = "declared somewhere in the main file";
@@ -281,8 +295,9 @@
   };
 
   // Diagnostics should turn into these:
-  clangd::Diagnostic MainLSP =
-  MatchingLSP(D, R"(Something terrible happened (fix available)
+  clangd::Diagnostic MainLSP = MatchingLSP(
+  D, R"(In file a/c.h:2:2: something terrible happened (fix available)
+In file included from: a/b.h:1:2
 
 main.cpp:6:7: remark: declared somewhere in the main file
 
@@ -603,7 +618,144 @@
   "Add include \"x.h\" for symbol a::X");
 }
 
+TEST(DiagsInHeaders, DiagInsideHeader) {
+  Annotations Main(R"cpp(
+#include [["a.h"]]
+void foo() {})cpp");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.AdditionalFiles = {{testPath("a.h"), "no_type_spec;"}};
+  EXPECT_THAT(TU.build().getDiagnostics(),
+  UnorderedElementsAre(
+  Diag(Main.range(),
+   "C++ requires a type specifier for all declarations",
+   std::vector>{
+   {"/clangd-test/a.h", pos(0, 0)}})));
+}
+
+TEST(DiagsInHeaders, DiagInTransitiveInclude) {
+  Annotations Main(R"cpp(
+#include [["a.h"]]
+void foo() {})

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-04-16 Thread Ilya Biryukov via Phabricator via cfe-commits

ilya-biryukov added inline comments.



Comment at: clangd/ClangdUnit.cpp:251
+  auto Res =
+  std::lower_bound(MainFileIncludes.begin(), MainFileIncludes.end(), Inc,
+   [](const Inclusion &LHS, const Inclusion &RHS) {

NIT: use `llvm::lower_bound`



Comment at: clangd/ClangdUnit.cpp:252
+  std::lower_bound(MainFileIncludes.begin(), MainFileIncludes.end(), Inc,
+   [](const Inclusion &LHS, const Inclusion &RHS) {
+ return LHS.R.start.line < RHS.R.start.line;

NIT: `lower_bound` also works when you work on a different type, no need for 
the temporary variable.
```
lower_bound(MainFileIncludes…, [](const Inclusion &L, const Position &Pos) {
  return L.start.line < Pos.line;
})
```

(the order of lamda parameters might need to be reversed, I always forget 
what's the correct one)



Comment at: clangd/ClangdUnit.cpp:255
+   });
+  return Res->R;
+}

NIT: add a sanity check that the include was actually there: `assert(Res->R == 
Pos)` 



Comment at: clangd/Diagnostics.cpp:55
+  }
+  if (!IncludeStack.empty()) {
+D.File = SM.getFileEntryForID(SM.getMainFileID())->getName().str();

reduce nesting by inverting the condition:
```
if (IncludeStack.empty())
  return;
// rest of the code
```



Comment at: clangd/Diagnostics.cpp:205
+  if (I != E - 1)
+OS << "In file included from: ";
+  OS << D.IncludeStack[I] << ": ";

Could we get the message first and the include stack later?
The first line is often showed in various lists and having `In file included 
from` there is not very helpful.



Comment at: clangd/Diagnostics.cpp:463
+  // header.
+  auto ShouldAddDiag = [this](const Diag &D) {
+if (mentionsMainFile(D))

Could you inline `ShouldAddDiag` into its single callsite?



Comment at: clangd/Diagnostics.h:87
+  /// file is in front.
+  std::vector IncludeStack;
 };

Could you store `pair` instead?
Would make it simpler to adapt D60267 to this change.



Comment at: clangd/Diagnostics.h:131
   llvm::Optional LastDiag;
+  // Counts diagnostics coming from a header in the main file.
+  llvm::DenseMap NumberOfDiagsAtLine;

NIT: use tripple slash comments



Comment at: clangd/Diagnostics.h:132
+  // Counts diagnostics coming from a header in the main file.
+  llvm::DenseMap NumberOfDiagsAtLine;
 };

NIT: the name makes me believe this does a different thing. Maybe mention 
"include" in the name to make it less confusing?
Something like `DiagsInsideIncludes` would work.

Another NIT: we don't need a map anymore: `DenseSet` should be enough



Comment at: unittests/clangd/DiagnosticsTests.cpp:54
+return false;
+  for (size_t I = 0, E = IncludeStack.size(); I < E; ++I)
+if (IncludeStack[I] != arg.IncludeStack[I])

NIT: maybe simplify to `std::equal(IncludeStack.begin(), IncludeStack.end(), 
arg.IncludeStack.begin())`



Comment at: unittests/clangd/DiagnosticsTests.cpp:620
 
+ParsedAST build(const std::string &MainFile,
+const llvm::StringMap &Files) {

We were discussing adding the extra files to `TestTU` instead.
Any reason this did not work out?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302



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

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-04-16 Thread Ilya Biryukov via Phabricator via cfe-commits

ilya-biryukov added a comment.

Sorry, lost this revision. Looking now


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302



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

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-04-16 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet added a comment.

Ping


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302



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

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-04-10 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet updated this revision to Diff 194528.
kadircet marked 10 inline comments as done.
kadircet added a comment.

- Address comments


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302

Files:
  clangd/ClangdUnit.cpp
  clangd/Diagnostics.cpp
  clangd/Diagnostics.h
  unittests/clangd/DiagnosticsTests.cpp

Index: unittests/clangd/DiagnosticsTests.cpp
===
--- unittests/clangd/DiagnosticsTests.cpp
+++ unittests/clangd/DiagnosticsTests.cpp
@@ -9,6 +9,7 @@
 #include "Annotations.h"
 #include "ClangdUnit.h"
 #include "SourceCode.h"
+#include "TestFS.h"
 #include "TestIndex.h"
 #include "TestTU.h"
 #include "index/MemIndex.h"
@@ -45,6 +46,17 @@
   return arg.Range == Range && arg.Message == Message;
 }
 
+MATCHER_P3(Diag, Range, Message, IncludeStack,
+   "Diag at " + llvm::to_string(Range) + " = [" + Message + "]") {
+  if (arg.Range != Range || arg.Message != Message ||
+  arg.IncludeStack.size() != IncludeStack.size())
+return false;
+  for (size_t I = 0, E = IncludeStack.size(); I < E; ++I)
+if (IncludeStack[I] != arg.IncludeStack[I])
+  return false;
+  return true;
+}
+
 MATCHER_P3(Fix, Range, Replacement, Message,
"Fix " + llvm::to_string(Range) + " => " +
testing::PrintToString(Replacement) + " = [" + Message + "]") {
@@ -73,7 +85,6 @@
   return true;
 }
 
-
 // Helper function to make tests shorter.
 Position pos(int line, int character) {
   Position Res;
@@ -251,6 +262,8 @@
   D.InsideMainFile = true;
   D.Severity = DiagnosticsEngine::Error;
   D.File = "foo/bar/main.cpp";
+  D.IncludeStack.push_back("a/b.h:1:2");
+  D.IncludeStack.push_back("a/c.h:2:2");
 
   clangd::Note NoteInMain;
   NoteInMain.Message = "declared somewhere in the main file";
@@ -282,7 +295,8 @@
 
   // Diagnostics should turn into these:
   clangd::Diagnostic MainLSP =
-  MatchingLSP(D, R"(Something terrible happened (fix available)
+  MatchingLSP(D, R"(In file included from: a/b.h:1:2: 
+a/c.h:2:2: something terrible happened (fix available)
 
 main.cpp:6:7: remark: declared somewhere in the main file
 
@@ -603,7 +617,185 @@
   "Add include \"x.h\" for symbol a::X");
 }
 
+ParsedAST build(const std::string &MainFile,
+const llvm::StringMap &Files) {
+  std::vector Cmd = {"clang", MainFile.c_str()};
+  ParseInputs Inputs;
+  Inputs.CompileCommand.Filename = MainFile;
+  Inputs.CompileCommand.CommandLine = {Cmd.begin(), Cmd.end()};
+  Inputs.CompileCommand.Directory = testRoot();
+  Inputs.Contents = Files.lookup(MainFile);
+  Inputs.FS = buildTestFS(Files);
+  Inputs.Opts = ParseOptions();
+  auto PCHs = std::make_shared();
+  auto CI = buildCompilerInvocation(Inputs);
+  assert(CI && "Failed to build compilation invocation.");
+  auto Preamble =
+  buildPreamble(MainFile, *CI,
+/*OldPreamble=*/nullptr,
+/*OldCompileCommand=*/Inputs.CompileCommand, Inputs,
+/*StoreInMemory=*/true, /*PreambleCallback=*/nullptr);
+  auto AST = buildAST(MainFile, createInvocationFromCommandLine(Cmd), Inputs,
+  Preamble);
+  if (!AST.hasValue()) {
+ADD_FAILURE() << "Failed to build code:\n" << Files.lookup(MainFile);
+llvm_unreachable("Failed to build TestTU!");
+  }
+  return std::move(*AST);
+}
+
+TEST(DiagsInHeaders, DiagInsideHeader) {
+  Annotations Main(R"cpp(
+#include [["a.h"]]
+void foo() {})cpp");
+  std::string MainFile = testPath("main.cc");
+  llvm::StringMap Files = {{MainFile, Main.code()},
+{testPath("a.h"), "no_type_spec;"}};
+  auto FS = buildTestFS(Files);
+
+  EXPECT_THAT(build(MainFile, Files).getDiagnostics(),
+  UnorderedElementsAre(
+  Diag(Main.range(),
+   "C++ requires a type specifier for all declarations",
+   std::vector{"/clangd-test/a.h:1:1"})));
+}
+
+TEST(DiagsInHeaders, DiagInTransitiveInclude) {
+  Annotations Main(R"cpp(
+#include [["a.h"]]
+void foo() {})cpp");
+  std::string MainFile = testPath("main.cc");
+  llvm::StringMap Files = {{MainFile, Main.code()},
+{testPath("a.h"), "#include \"b.h\""},
+{testPath("b.h"), "no_type_spec;"}};
+  auto FS = buildTestFS(Files);
+
+  EXPECT_THAT(build(MainFile, Files).getDiagnostics(),
+  UnorderedElementsAre(
+  Diag(Main.range(),
+   "C++ requires a type specifier for all declarations",
+   std::vector{"/clangd-test/a.h:1:10",
+"/clangd-test/b.h:1:1"})));
+}
+
+TEST(DiagsInHeaders, DiagInMultipleHeaders) {
+  Annotations Main(R"cpp(
+#include $a[["a.h"]]
+#include $b[["b.h"]]
+void foo() {})cpp");
+

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-04-10 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet added inline comments.



Comment at: clangd/Diagnostics.cpp:396
+  for (llvm::StringRef Inc : IncludeStack)
+OS << "In file included from: " << Inc << ":\n";
+}

ilya-biryukov wrote:
> kadircet wrote:
> > ilya-biryukov wrote:
> > > NIT: could we reuse the function from clang that does the same thing?
> > > 
> > > The code here is pretty simple, though, so writing it here is fine. Just 
> > > wanted to make sure we considered this option and found it's easier to 
> > > redo this work ourselves.
> > there is `TextDiagnostic` which prints the desired output expect the fact 
> > that it also prints the first line saying the header was included in main 
> > file. Therefore, I didn't make use of it since we decided that was not very 
> > useful for our use case. And it requires inheriting from 
> > `DiagnosticRenderer` which was requiring too much boiler plate code just to 
> > get this functionality so instead I've chosen doing it like this.
> > 
> > Can fallback to `TextDiagnostic` if you believe showing `Included in 
> > mainfile.cc:x:y:` at the beginning of the diagnostic won't be annoying.
> LG, it's not too hard to reconstruct the same output.
> Note that D60267 starts outputting notes in a structured way, using the 
> `RelatedInformation` struct from the LSP.
> 
> We should eventually add the include stack into related information too. With 
> that in mind, we should probably add the include stack as a new field to the 
> struct we use for diagnostics.
> 
SG, delaying serialization to LSP layer then.



Comment at: clangd/Diagnostics.cpp:372
+auto IncludeLocation = Info.getSourceManager()
+   .getPresumedLoc(Info.getLocation(),
+   /*UseLineDirectives=*/false)

ilya-biryukov wrote:
> Why use `getPresumedLoc`? Making clangd sensitive to pp directives that 
> rename file or change line numbers does not make any sense.
> 
> Could you also add tests for that?
It was the way `DiagnosticRenderer` emitted include stacks, I had just copied 
it over.
Changing with `SourceManger::getIncludeLoc`


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302



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

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-04-07 Thread Ilya Biryukov via Phabricator via cfe-commits

ilya-biryukov added inline comments.

Comment at: clangd/ClangdUnit.cpp:405

-  std::vector Diags = ASTDiags.take();
+  auto ShouldSurfaceDiag = [](const Diag &D) {
+if (D.Severity == DiagnosticsEngine::Level::Error ||

The name suggests we filter **all** diagnostics based on this helper.
Maybe add rename to something more specific?

E.g. `IsErrorOrFatal` or `IsImporantHeaderDiagnostic`...

Comment at: clangd/ClangdUnit.cpp:408
+D.Severity == DiagnosticsEngine::Level::Fatal)
+  return true;
+return false;

NIT: simplify to 
```
return D.Severity == DiagnosticsEngine::Level::Error
|| D.Severity == DiagnosticsEngine::Level::Fatal
```

Comment at: clangd/ClangdUnit.cpp:414
+  llvm::DenseMap NumberOfDiagsAtLine;
+  auto TryAddDiag = [&Diags, &NumberOfDiagsAtLine](const Diag &D) {
+if (++NumberOfDiagsAtLine[D.Range.start.line] > 1) {

NIT: rename to `AddDiagnosticFromInclude` or something similar, but more 
specific than the current name

Comment at: clangd/ClangdUnit.cpp:425
+  Diags.push_back(D);
+else if (ShouldSurfaceDiag(D))
+  TryAddDiag(D);

Why not merge the `ShouldSurfaceDiag` and `TryAddDiag` into a single function 
and handle all the complexities there?
This would simplify the client code, it would be as simple as 
```
else
 AddDiagnosticFromInclude(D)
```

Comment at: clangd/ClangdUnit.cpp:434
+  if (Preamble) {
+for (const Diag &D : Preamble->Diags) {
+  if (mentionsMainFile(D))

Can we do this in `StoreDiags::flushLastDiag`?
All code handling the diagnostics lives there and it has all the information 
necessary to map to the included line.

Comment at: clangd/ClangdUnit.cpp:445
+  // same line.
+  for (auto &D : Diags) {
+if (!mentionsMainFile(D)) {

This extra complexity does not seem to be worth it after all.
I'd suggest to remove this (at least from the first version of the file), even 
though I was the one who proposed it.

Still think it's valuable, but the patch is complicated enough as it is, 
keeping it simpler seems to be more important at this point.

Comment at: clangd/Diagnostics.cpp:396
+  for (llvm::StringRef Inc : IncludeStack)
+OS << "In file included from: " << Inc << ":\n";
+}

kadircet wrote:
> ilya-biryukov wrote:
> > NIT: could we reuse the function from clang that does the same thing?
> > 
> > The code here is pretty simple, though, so writing it here is fine. Just 
> > wanted to make sure we considered this option and found it's easier to redo 
> > this work ourselves.
> there is `TextDiagnostic` which prints the desired output expect the fact 
> that it also prints the first line saying the header was included in main 
> file. Therefore, I didn't make use of it since we decided that was not very 
> useful for our use case. And it requires inheriting from `DiagnosticRenderer` 
> which was requiring too much boiler plate code just to get this functionality 
> so instead I've chosen doing it like this.
> 
> Can fallback to `TextDiagnostic` if you believe showing `Included in 
> mainfile.cc:x:y:` at the beginning of the diagnostic won't be annoying.
LG, it's not too hard to reconstruct the same output.
Note that D60267 starts outputting notes in a structured way, using the 
`RelatedInformation` struct from the LSP.

We should eventually add the include stack into related information too. With 
that in mind, we should probably add the include stack as a new field to the 
struct we use for diagnostics.

Comment at: clangd/Diagnostics.cpp:371
 FillDiagBase(*LastDiag);
+auto IncludeLocation = Info.getSourceManager()
+   .getPresumedLoc(Info.getLocation(),

That's a lot of code, could we extract this into a separate function?

Comment at: clangd/Diagnostics.cpp:372
+auto IncludeLocation = Info.getSourceManager()
+   .getPresumedLoc(Info.getLocation(),
+   /*UseLineDirectives=*/false)

Why use `getPresumedLoc`? Making clangd sensitive to pp directives that rename 
file or change line numbers does not make any sense.

Could you also add tests for that?

Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302

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

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-03-22 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet updated this revision to Diff 191831.
kadircet marked 8 inline comments as done.
kadircet added a comment.

- Address comments


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302

Files:
  clangd/ClangdUnit.cpp
  clangd/Diagnostics.cpp
  clangd/Diagnostics.h
  unittests/clangd/DiagnosticsTests.cpp

Index: unittests/clangd/DiagnosticsTests.cpp
===
--- unittests/clangd/DiagnosticsTests.cpp
+++ unittests/clangd/DiagnosticsTests.cpp
@@ -9,6 +9,7 @@
 #include "Annotations.h"
 #include "ClangdUnit.h"
 #include "SourceCode.h"
+#include "TestFS.h"
 #include "TestIndex.h"
 #include "TestTU.h"
 #include "index/MemIndex.h"
@@ -73,7 +74,6 @@
   return true;
 }
 
-
 // Helper function to make tests shorter.
 Position pos(int line, int character) {
   Position Res;
@@ -603,7 +603,183 @@
   "Add include \"x.h\" for symbol a::X");
 }
 
+ParsedAST build(const std::string &MainFile,
+const llvm::StringMap &Files) {
+  std::vector Cmd = {"clang", MainFile.c_str()};
+  ParseInputs Inputs;
+  Inputs.CompileCommand.Filename = MainFile;
+  Inputs.CompileCommand.CommandLine = {Cmd.begin(), Cmd.end()};
+  Inputs.CompileCommand.Directory = testRoot();
+  Inputs.Contents = Files.lookup(MainFile);
+  Inputs.FS = buildTestFS(Files);
+  Inputs.Opts = ParseOptions();
+  auto PCHs = std::make_shared();
+  auto CI = buildCompilerInvocation(Inputs);
+  assert(CI && "Failed to build compilation invocation.");
+  auto Preamble =
+  buildPreamble(MainFile, *CI,
+/*OldPreamble=*/nullptr,
+/*OldCompileCommand=*/Inputs.CompileCommand, Inputs, PCHs,
+/*StoreInMemory=*/true, /*PreambleCallback=*/nullptr);
+  auto AST = buildAST(MainFile, createInvocationFromCommandLine(Cmd), Inputs,
+  Preamble, PCHs);
+  if (!AST.hasValue()) {
+ADD_FAILURE() << "Failed to build code:\n" << Files.lookup(MainFile);
+llvm_unreachable("Failed to build TestTU!");
+  }
+  return std::move(*AST);
+}
+
+TEST(DiagsInHeaders, DiagInsideHeader) {
+  Annotations Main(R"cpp(
+#include [["a.h"]]
+void foo() {})cpp");
+  std::string MainFile = testPath("main.cc");
+  llvm::StringMap Files = {{MainFile, Main.code()},
+{testPath("a.h"), "no_type_spec;"}};
+  auto FS = buildTestFS(Files);
+
+  EXPECT_THAT(build(MainFile, Files).getDiagnostics(),
+  UnorderedElementsAre(
+  Diag(Main.range(), "/clangd-test/a.h:1:1: C++ requires a "
+ "type specifier for all declarations")));
+}
+
+TEST(DiagsInHeaders, DiagInTransitiveInclude) {
+  Annotations Main(R"cpp(
+#include [["a.h"]]
+void foo() {})cpp");
+  std::string MainFile = testPath("main.cc");
+  llvm::StringMap Files = {{MainFile, Main.code()},
+{testPath("a.h"), "#include \"b.h\""},
+{testPath("b.h"), "no_type_spec;"}};
+  auto FS = buildTestFS(Files);
+
+  EXPECT_THAT(build(MainFile, Files).getDiagnostics(),
+  UnorderedElementsAre(
+  Diag(Main.range(),
+   "In file included from: "
+   "/clangd-test/a.h:1:10:\n/clangd-test/b.h:1:1: C++ "
+   "requires a type specifier for all declarations")));
+}
+
+TEST(DiagsInHeaders, DiagInMultipleHeaders) {
+  Annotations Main(R"cpp(
+#include $a[["a.h"]]
+#include $b[["b.h"]]
+void foo() {})cpp");
+  std::string MainFile = testPath("main.cc");
+  llvm::StringMap Files = {{MainFile, Main.code()},
+{testPath("a.h"), "no_type_spec;"},
+{testPath("b.h"), "no_type_spec;"}};
+  auto FS = buildTestFS(Files);
+
+  EXPECT_THAT(
+  build(MainFile, Files).getDiagnostics(),
+  UnorderedElementsAre(
+  Diag(Main.range("a"), "/clangd-test/a.h:1:1: C++ requires a type "
+"specifier for all declarations"),
+  Diag(Main.range("b"), "/clangd-test/b.h:1:1: C++ requires a type "
+"specifier for all declarations")));
+}
+
+TEST(DiagsInHeaders, PreferExpansionLocation) {
+  Annotations Main(R"cpp(
+#include [["a.h"]]
+#include "b.h"
+void foo() {})cpp");
+  std::string MainFile = testPath("main.cc");
+  llvm::StringMap Files = {
+  {MainFile, Main.code()},
+  {testPath("a.h"), "#include \"b.h\"\n"},
+  {testPath("b.h"), "#ifndef X\n#define X\nno_type_spec;\n#endif"}};
+  auto FS = buildTestFS(Files);
+
+  EXPECT_THAT(build(MainFile, Files).getDiagnostics(),
+  UnorderedElementsAre(
+  Diag(Main.range(),
+   "In file included from: "
+   "/clangd-test/a.h:1:10:\n/clangd-

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-03-22 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet added inline comments.



Comment at: clangd/Diagnostics.cpp:396
+  for (llvm::StringRef Inc : IncludeStack)
+OS << "In file included from: " << Inc << ":\n";
+}

ilya-biryukov wrote:
> NIT: could we reuse the function from clang that does the same thing?
> 
> The code here is pretty simple, though, so writing it here is fine. Just 
> wanted to make sure we considered this option and found it's easier to redo 
> this work ourselves.
there is `TextDiagnostic` which prints the desired output expect the fact that 
it also prints the first line saying the header was included in main file. 
Therefore, I didn't make use of it since we decided that was not very useful 
for our use case. And it requires inheriting from `DiagnosticRenderer` which 
was requiring too much boiler plate code just to get this functionality so 
instead I've chosen doing it like this.

Can fallback to `TextDiagnostic` if you believe showing `Included in 
mainfile.cc:x:y:` at the beginning of the diagnostic won't be annoying.



Comment at: unittests/clangd/DiagnosticsTests.cpp:779
 } // namespace clangd
 } // namespace clang

ilya-biryukov wrote:
> Could we add a test for reaching the limit?
> Could we mention that some diagnostics were dropped in that case? Something 
> like:
> ```
> In file included from a.h:20:1:
> error: unresolved identifier 'foo'.
> 
> And 10 more diagnostics...
> ```
There is already one, see `LimitDiagsOutsideMainFile`


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302



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

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-03-20 Thread Ilya Biryukov via Phabricator via cfe-commits

ilya-biryukov added a comment.

Thanks for changes. A few more comments.




Comment at: clangd/ClangdUnit.cpp:259
+// Generates a mapping from start of an include directive to its range.
+std::map
+positionToRangeMap(const std::vector &MainFileIncludes) {

Could we binary-search in a sorted `vector` with a custom comparator 
instead?



Comment at: clangd/ClangdUnit.cpp:411
+MaxDiagsPerIncludeDirective) {
+  ++NumberOfDiagsAtLine[D.IncludeDirective->line];
+  return true;

The function name suggests it does not have side-effects.
Maybe handle the update and query of `NumberOfDiagstAtLine` outside this 
function instead?



Comment at: clangd/Diagnostics.cpp:396
+  for (llvm::StringRef Inc : IncludeStack)
+OS << "In file included from: " << Inc << ":\n";
+}

NIT: could we reuse the function from clang that does the same thing?

The code here is pretty simple, though, so writing it here is fine. Just wanted 
to make sure we considered this option and found it's easier to redo this work 
ourselves.



Comment at: clangd/Diagnostics.cpp:419
-  else
-vlog("Dropped diagnostic outside main file: {0}: {1}", LastDiag->File,
- LastDiag->Message);

We can still drop diagnostics at some point, right?
Could we move the log statement in a different point?




Comment at: clangd/Diagnostics.h:87
+  /// directive, otherwise None.
+  llvm::Optional IncludeDirective = llvm::None;
 };

Could we avoid changing this interface?
We have enough information to fill the corresponding range and message when 
collecting diagnostics inside our implementation of clang's 
`DiagnosticConsumer`, there's really no point in carrying this information in 
this struct.

If that means intermediate structures stored in our consumer, so be it, it's a 
private implementation detail. `Diag` is a public interface, so keeping it 
simpler is valuable.



Comment at: unittests/clangd/DiagnosticsTests.cpp:773
+  UnorderedElementsAre(
+  Diag(Main.range(), "/clangd-test/a.h:2:44: C++ requires a "
+ "type specifier for all declarations")));

NIT: maybe use raw string literals to keep the message a one-liner?



Comment at: unittests/clangd/DiagnosticsTests.cpp:779
 } // namespace clangd
 } // namespace clang

Could we add a test for reaching the limit?
Could we mention that some diagnostics were dropped in that case? Something 
like:
```
In file included from a.h:20:1:
error: unresolved identifier 'foo'.

And 10 more diagnostics...
```


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302



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

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-03-20 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet updated this revision to Diff 191465.
kadircet marked an inline comment as done.
kadircet added a comment.

- Make limit a hardcoded value rather then a command line argument
- Limit diags per include directive


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302

Files:
  clangd/ClangdUnit.cpp
  clangd/Diagnostics.cpp
  clangd/Diagnostics.h
  unittests/clangd/DiagnosticsTests.cpp

Index: unittests/clangd/DiagnosticsTests.cpp
===
--- unittests/clangd/DiagnosticsTests.cpp
+++ unittests/clangd/DiagnosticsTests.cpp
@@ -9,6 +9,7 @@
 #include "Annotations.h"
 #include "ClangdUnit.h"
 #include "SourceCode.h"
+#include "TestFS.h"
 #include "TestIndex.h"
 #include "TestTU.h"
 #include "index/MemIndex.h"
@@ -73,7 +74,6 @@
   return true;
 }
 
-
 // Helper function to make tests shorter.
 Position pos(int line, int character) {
   Position Res;
@@ -603,7 +603,177 @@
   "Add include \"x.h\" for symbol a::X");
 }
 
+ParsedAST build(const std::string &MainFile,
+const llvm::StringMap &Files) {
+  std::vector Cmd = {"clang", MainFile.c_str()};
+  ParseInputs Inputs;
+  Inputs.CompileCommand.Filename = MainFile;
+  Inputs.CompileCommand.CommandLine = {Cmd.begin(), Cmd.end()};
+  Inputs.CompileCommand.Directory = testRoot();
+  Inputs.Contents = Files.lookup(MainFile);
+  Inputs.FS = buildTestFS(Files);
+  Inputs.Opts = ParseOptions();
+  auto PCHs = std::make_shared();
+  auto CI = buildCompilerInvocation(Inputs);
+  assert(CI && "Failed to build compilation invocation.");
+  auto Preamble =
+  buildPreamble(MainFile, *CI,
+/*OldPreamble=*/nullptr,
+/*OldCompileCommand=*/Inputs.CompileCommand, Inputs, PCHs,
+/*StoreInMemory=*/true, /*PreambleCallback=*/nullptr);
+  auto AST = buildAST(MainFile, createInvocationFromCommandLine(Cmd), Inputs,
+  Preamble, PCHs);
+  if (!AST.hasValue()) {
+ADD_FAILURE() << "Failed to build code:\n" << Files.lookup(MainFile);
+llvm_unreachable("Failed to build TestTU!");
+  }
+  return std::move(*AST);
+}
+
+TEST(DiagsInHeaders, DiagInsideHeader) {
+  Annotations Main(R"cpp(
+#include [["a.h"]]
+void foo() {})cpp");
+  std::string MainFile = testPath("main.cc");
+  llvm::StringMap Files = {{MainFile, Main.code()},
+{testPath("a.h"), "no_type_spec;"}};
+  auto FS = buildTestFS(Files);
+
+  EXPECT_THAT(build(MainFile, Files).getDiagnostics(),
+  UnorderedElementsAre(
+  Diag(Main.range(), "/clangd-test/a.h:1:1: C++ requires a "
+ "type specifier for all declarations")));
+}
+
+TEST(DiagsInHeaders, DiagInTransitiveInclude) {
+  Annotations Main(R"cpp(
+#include [["a.h"]]
+void foo() {})cpp");
+  std::string MainFile = testPath("main.cc");
+  llvm::StringMap Files = {{MainFile, Main.code()},
+{testPath("a.h"), "#include \"b.h\""},
+{testPath("b.h"), "no_type_spec;"}};
+  auto FS = buildTestFS(Files);
+
+  EXPECT_THAT(build(MainFile, Files).getDiagnostics(),
+  UnorderedElementsAre(
+  Diag(Main.range(),
+   "In file included from: "
+   "/clangd-test/a.h:1:10:\n/clangd-test/b.h:1:1: C++ "
+   "requires a type specifier for all declarations")));
+}
+
+TEST(DiagsInHeaders, DiagInMultipleHeaders) {
+  Annotations Main(R"cpp(
+#include $a[["a.h"]]
+#include $b[["b.h"]]
+void foo() {})cpp");
+  std::string MainFile = testPath("main.cc");
+  llvm::StringMap Files = {{MainFile, Main.code()},
+{testPath("a.h"), "no_type_spec;"},
+{testPath("b.h"), "no_type_spec;"}};
+  auto FS = buildTestFS(Files);
+
+  EXPECT_THAT(
+  build(MainFile, Files).getDiagnostics(),
+  UnorderedElementsAre(
+  Diag(Main.range("a"), "/clangd-test/a.h:1:1: C++ requires a type "
+"specifier for all declarations"),
+  Diag(Main.range("b"), "/clangd-test/b.h:1:1: C++ requires a type "
+"specifier for all declarations")));
+}
+
+TEST(DiagsInHeaders, PreferExpansionLocation) {
+  Annotations Main(R"cpp(
+#include [["a.h"]]
+#include "b.h"
+void foo() {})cpp");
+  std::string MainFile = testPath("main.cc");
+  llvm::StringMap Files = {
+  {MainFile, Main.code()},
+  {testPath("a.h"), "#include \"b.h\"\n"},
+  {testPath("b.h"), "#ifndef X\n#define X\nno_type_spec;\n#endif"}};
+  auto FS = buildTestFS(Files);
+
+  EXPECT_THAT(build(MainFile, Files).getDiagnostics(),
+  UnorderedElementsAre(
+  Diag(Main.range(),
+

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-03-19 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet planned changes to this revision.
kadircet added a comment.

- Limit per include directive
- Use hardcoded value for limit


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302



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

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-03-19 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet updated this revision to Diff 191323.
kadircet marked 2 inline comments as done.
kadircet added a comment.

- Show include stack in diagnostic message
- Point to exact include location


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/ClangdUnit.cpp
  clangd/Compiler.h
  clangd/Diagnostics.cpp
  clangd/Diagnostics.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/DiagnosticsTests.cpp

Index: unittests/clangd/DiagnosticsTests.cpp
===
--- unittests/clangd/DiagnosticsTests.cpp
+++ unittests/clangd/DiagnosticsTests.cpp
@@ -9,6 +9,7 @@
 #include "Annotations.h"
 #include "ClangdUnit.h"
 #include "SourceCode.h"
+#include "TestFS.h"
 #include "TestIndex.h"
 #include "TestTU.h"
 #include "index/MemIndex.h"
@@ -73,7 +74,6 @@
   return true;
 }
 
-
 // Helper function to make tests shorter.
 Position pos(int line, int character) {
   Position Res;
@@ -603,7 +603,174 @@
   "Add include \"x.h\" for symbol a::X");
 }
 
+ParsedAST
+build(const std::string &MainFile, const llvm::StringMap &Files,
+  llvm::Optional LimitDiagsOutsideMainFile = llvm::None) {
+  std::vector Cmd = {"clang", MainFile.c_str()};
+  ParseInputs Inputs;
+  Inputs.CompileCommand.Filename = MainFile;
+  Inputs.CompileCommand.CommandLine = {Cmd.begin(), Cmd.end()};
+  Inputs.CompileCommand.Directory = testRoot();
+  Inputs.Contents = Files.lookup(MainFile);
+  Inputs.FS = buildTestFS(Files);
+  Inputs.Opts = ParseOptions();
+  Inputs.Opts.LimitDiagsOutsideMainFile = LimitDiagsOutsideMainFile;
+  auto PCHs = std::make_shared();
+  auto CI = buildCompilerInvocation(Inputs);
+  assert(CI && "Failed to build compilation invocation.");
+  auto Preamble =
+  buildPreamble(MainFile, *CI,
+/*OldPreamble=*/nullptr,
+/*OldCompileCommand=*/Inputs.CompileCommand, Inputs, PCHs,
+/*StoreInMemory=*/true, /*PreambleCallback=*/nullptr);
+  auto AST = buildAST(MainFile, createInvocationFromCommandLine(Cmd), Inputs,
+  Preamble, PCHs);
+  if (!AST.hasValue()) {
+ADD_FAILURE() << "Failed to build code:\n" << Files.lookup(MainFile);
+llvm_unreachable("Failed to build TestTU!");
+  }
+  return std::move(*AST);
+}
+
+TEST(DiagsInHeaders, DiagInsideHeader) {
+  Annotations Main(R"cpp(
+#include [["a.h"]]
+void foo() {})cpp");
+  std::string MainFile = testPath("main.cc");
+  llvm::StringMap Files = {{MainFile, Main.code()},
+{testPath("a.h"), "no_type_spec;"}};
+  auto FS = buildTestFS(Files);
+
+  EXPECT_THAT(build(MainFile, Files).getDiagnostics(),
+  UnorderedElementsAre(
+  Diag(Main.range(), "/clangd-test/a.h:1:1: C++ requires a "
+ "type specifier for all declarations")));
+}
+
+TEST(DiagsInHeaders, DiagInTransitiveInclude) {
+  Annotations Main(R"cpp(
+#include [["a.h"]]
+void foo() {})cpp");
+  std::string MainFile = testPath("main.cc");
+  llvm::StringMap Files = {{MainFile, Main.code()},
+{testPath("a.h"), "#include \"b.h\""},
+{testPath("b.h"), "no_type_spec;"}};
+  auto FS = buildTestFS(Files);
+
+  EXPECT_THAT(build(MainFile, Files).getDiagnostics(),
+  UnorderedElementsAre(
+  Diag(Main.range(),
+   "In file included from: "
+   "/clangd-test/a.h:1:10:\n/clangd-test/b.h:1:1: C++ "
+   "requires a type specifier for all declarations")));
+}
+
+TEST(DiagsInHeaders, DiagInMultipleHeaders) {
+  Annotations Main(R"cpp(
+#include $a[["a.h"]]
+#include $b[["b.h"]]
+void foo() {})cpp");
+  std::string MainFile = testPath("main.cc");
+  llvm::StringMap Files = {{MainFile, Main.code()},
+{testPath("a.h"), "no_type_spec;"},
+{testPath("b.h"), "no_type_spec;"}};
+  auto FS = buildTestFS(Files);
+
+  EXPECT_THAT(
+  build(MainFile, Files).getDiagnostics(),
+  UnorderedElementsAre(
+  Diag(Main.range("a"), "/clangd-test/a.h:1:1: C++ requires a type "
+"specifier for all declarations"),
+  Diag(Main.range("b"), "/clangd-test/b.h:1:1: C++ requires a type "
+"specifier for all declarations")));
+}
+
+TEST(DiagsInHeaders, PreferExpansionLocation) {
+  Annotations Main(R"cpp(
+#include [["a.h"]]
+#include "b.h"
+void foo() {})cpp");
+  std::string MainFile = testPath("main.cc");
+  llvm::StringMap Files = {
+  {MainFile, Main.code()},
+  {testPath("a.h"), "#include \"b.h\"\n"},
+  {testPath("b.h"), "#ifndef X\n#define X\nno_type_spec;\n#endif"}};

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-03-19 Thread Ilya Biryukov via Phabricator via cfe-commits

ilya-biryukov added inline comments.



Comment at: clangd/ClangdUnit.cpp:256
+// directive inside main file.
+// If a header is transitively included in multiple direct includes of main, we
+// choose the first one.

We should find a way to point into an exact `include` directive (source 
locations have this information).




Comment at: clangd/ClangdUnit.cpp:406
+  for (const Diag &D : ASTDiags.take()) {
+if (!mentionsMainFile(D) && Opts.LimitDiagsOutsideMainFile &&
+++DiagsOutsideMainFile > *Opts.LimitDiagsOutsideMainFile)

Could we limit per-include-directive instead?
The limit to avoid creating too verbose error messages, rather than reporting 
too many messages, right?



Comment at: clangd/ClangdUnit.cpp:438
+  D.File = MainInput.getFile();
+  D.Message = "Error in header: " + D.Message;
+}

We should provide the include stack, similar to how compiler does it.
Otherwise it would be really hard to figure out the exact problem the 
diagnostic is pointing at.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302



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

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-03-19 Thread Ilya Biryukov via Phabricator via cfe-commits

ilya-biryukov added a comment.

In D59302#1431274 , @kadircet wrote:

> The optional part is rather limiting number of diagnostics that is going to 
> be surfaced. So if you have thousands of errors inside preamble only first 
> 100 of them will appear inside main file by default.
>  But looking at it again, currently there is no way for user to say "do not 
> limit number of diagnostics" :D They can only change this limit to different 
> values.

Let's pick a number and hard-code it. We'll get consistent behavior in all 
clients of clangd, I can hardly think of any existing client that would want to 
surface everything to the user.
FWIW, `100` seem too much to my taste, since we're only showing errors. Would 
`10` be a better limit?

Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302

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

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-03-19 Thread Kadir Cetinkaya via Phabricator via cfe-commits

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

- Only surface diagnostics with level fatal or error


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/ClangdUnit.cpp
  clangd/Compiler.h
  clangd/Diagnostics.cpp
  clangd/tool/ClangdMain.cpp
  unittests/clangd/DiagnosticsTests.cpp

Index: unittests/clangd/DiagnosticsTests.cpp
===
--- unittests/clangd/DiagnosticsTests.cpp
+++ unittests/clangd/DiagnosticsTests.cpp
@@ -9,6 +9,7 @@
 #include "Annotations.h"
 #include "ClangdUnit.h"
 #include "SourceCode.h"
+#include "TestFS.h"
 #include "TestIndex.h"
 #include "TestTU.h"
 #include "index/MemIndex.h"
@@ -73,7 +74,6 @@
   return true;
 }
 
-
 // Helper function to make tests shorter.
 Position pos(int line, int character) {
   Position Res;
@@ -603,7 +603,163 @@
   "Add include \"x.h\" for symbol a::X");
 }
 
+ParsedAST
+build(const std::string &MainFile, const llvm::StringMap &Files,
+  llvm::Optional LimitDiagsOutsideMainFile = llvm::None) {
+  std::vector Cmd = {"clang", MainFile.c_str()};
+  ParseInputs Inputs;
+  Inputs.CompileCommand.Filename = MainFile;
+  Inputs.CompileCommand.CommandLine = {Cmd.begin(), Cmd.end()};
+  Inputs.CompileCommand.Directory = testRoot();
+  Inputs.Contents = Files.lookup(MainFile);
+  Inputs.FS = buildTestFS(Files);
+  Inputs.Opts = ParseOptions();
+  Inputs.Opts.LimitDiagsOutsideMainFile = LimitDiagsOutsideMainFile;
+  auto PCHs = std::make_shared();
+  auto CI = buildCompilerInvocation(Inputs);
+  assert(CI && "Failed to build compilation invocation.");
+  auto Preamble =
+  buildPreamble(MainFile, *CI,
+/*OldPreamble=*/nullptr,
+/*OldCompileCommand=*/Inputs.CompileCommand, Inputs, PCHs,
+/*StoreInMemory=*/true, /*PreambleCallback=*/nullptr);
+  auto AST = buildAST(MainFile, createInvocationFromCommandLine(Cmd), Inputs,
+  Preamble, PCHs);
+  if (!AST.hasValue()) {
+ADD_FAILURE() << "Failed to build code:\n" << Files.lookup(MainFile);
+llvm_unreachable("Failed to build TestTU!");
+  }
+  return std::move(*AST);
+}
+
+TEST(DiagsInHeaders, DiagInsideHeader) {
+  Annotations Main(R"cpp(
+#include [["a.h"]]
+void foo() {})cpp");
+  std::string MainFile = testPath("main.cc");
+  llvm::StringMap Files = {{MainFile, Main.code()},
+{testPath("a.h"), "no_type_spec;"}};
+  auto FS = buildTestFS(Files);
+
+  EXPECT_THAT(build(MainFile, Files).getDiagnostics(),
+  UnorderedElementsAre(Diag(Main.range(),
+"Error in header: C++ requires a type "
+"specifier for all declarations")));
+}
+
+TEST(DiagsInHeaders, DiagInTransitiveInclude) {
+  Annotations Main(R"cpp(
+#include [["a.h"]]
+void foo() {})cpp");
+  std::string MainFile = testPath("main.cc");
+  llvm::StringMap Files = {{MainFile, Main.code()},
+{testPath("a.h"), "#include \"b.h\""},
+{testPath("b.h"), "no_type_spec;"}};
+  auto FS = buildTestFS(Files);
+
+  EXPECT_THAT(build(MainFile, Files).getDiagnostics(),
+  UnorderedElementsAre(Diag(Main.range(),
+"Error in header: C++ requires a type "
+"specifier for all declarations")));
+}
+
+TEST(DiagsInHeaders, DiagInMultipleHeaders) {
+  Annotations Main(R"cpp(
+#include $a[["a.h"]]
+#include $b[["b.h"]]
+void foo() {})cpp");
+  std::string MainFile = testPath("main.cc");
+  llvm::StringMap Files = {{MainFile, Main.code()},
+{testPath("a.h"), "no_type_spec;"},
+{testPath("b.h"), "no_type_spec;"}};
+  auto FS = buildTestFS(Files);
+
+  EXPECT_THAT(build(MainFile, Files).getDiagnostics(),
+  UnorderedElementsAre(
+  Diag(Main.range("a"), "Error in header: C++ requires a type "
+"specifier for all declarations"),
+  Diag(Main.range("b"), "Error in header: C++ requires a type "
+"specifier for all declarations")));
+}
+
+TEST(DiagsInHeaders, PreferDirectIncludeLocation) {
+  Annotations Main(R"cpp(
+#include "a.h"
+#include [["b.h"]]
+void foo() {})cpp");
+  std::string MainFile = testPath("main.cc");
+  llvm::StringMap Files = {
+  {MainFile, Main.code()},
+  {testPath("a.h"), "#include \"b.h\"\n"},
+  {testPath("b.h"), "#ifndef X\n#define X\nno_type_spec;\n#endif"}};
+  auto FS = buildTestFS(Files);
+
+  EXPECT_THAT(build(MainFile, Files).getDiagnostics(),
+  Unor

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-03-15 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet added a comment.

In D59302#1431045 , @ilya-biryukov 
wrote:

> I'll look more closely into the details, but just a high-level question now: 
> why would we want to make this optional and not simply surface these extra 
> diagnostics?

The optional part is rather limiting number of diagnostics that is going to be 
surfaced. So if you have thousands of errors inside preamble only first 100 of 
them will appear inside main file by default.
But looking at it again, currently there is no way for user to say "do not 
limit number of diagnostics" :D They can only change this limit to different 
values.

Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302

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

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-03-15 Thread Ilya Biryukov via Phabricator via cfe-commits

ilya-biryukov added a comment.

I'll look more closely into the details, but just a high-level question now: 
why would we want to make this optional and not simply surface these extra 
diagnostics?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59302



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

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

2019-03-13 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet created this revision.
kadircet added reviewers: ioeric, ilya-biryukov.
Herald added subscribers: cfe-commits, jdoerfert, arphaman, jkorous, MaskRay.
Herald added a project: clang.

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D59302

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/ClangdUnit.cpp
  clangd/Compiler.h
  clangd/Diagnostics.cpp
  clangd/tool/ClangdMain.cpp
  unittests/clangd/DiagnosticsTests.cpp

Index: unittests/clangd/DiagnosticsTests.cpp
===
--- unittests/clangd/DiagnosticsTests.cpp
+++ unittests/clangd/DiagnosticsTests.cpp
@@ -9,6 +9,7 @@
 #include "Annotations.h"
 #include "ClangdUnit.h"
 #include "SourceCode.h"
+#include "TestFS.h"
 #include "TestIndex.h"
 #include "TestTU.h"
 #include "index/MemIndex.h"
@@ -73,7 +74,6 @@
   return true;
 }
 
-
 // Helper function to make tests shorter.
 Position pos(int line, int character) {
   Position Res;
@@ -603,7 +603,144 @@
   "Add include \"x.h\" for symbol a::X");
 }
 
+ParsedAST
+build(const std::string &MainFile, const llvm::StringMap &Files,
+  llvm::Optional LimitDiagsOutsideMainFile = llvm::None) {
+  std::vector Cmd = {"clang", MainFile.c_str()};
+  ParseInputs Inputs;
+  Inputs.CompileCommand.Filename = MainFile;
+  Inputs.CompileCommand.CommandLine = {Cmd.begin(), Cmd.end()};
+  Inputs.CompileCommand.Directory = testRoot();
+  Inputs.Contents = Files.lookup(MainFile);
+  Inputs.FS = buildTestFS(Files);
+  Inputs.Opts = ParseOptions();
+  Inputs.Opts.LimitDiagsOutsideMainFile = LimitDiagsOutsideMainFile;
+  auto PCHs = std::make_shared();
+  auto CI = buildCompilerInvocation(Inputs);
+  assert(CI && "Failed to build compilation invocation.");
+  auto Preamble =
+  buildPreamble(MainFile, *CI,
+/*OldPreamble=*/nullptr,
+/*OldCompileCommand=*/Inputs.CompileCommand, Inputs, PCHs,
+/*StoreInMemory=*/true, /*PreambleCallback=*/nullptr);
+  auto AST = buildAST(MainFile, createInvocationFromCommandLine(Cmd), Inputs,
+  Preamble, PCHs);
+  if (!AST.hasValue()) {
+ADD_FAILURE() << "Failed to build code:\n" << Files.lookup(MainFile);
+llvm_unreachable("Failed to build TestTU!");
+  }
+  return std::move(*AST);
+}
+
+TEST(DiagsInHeaders, DiagInsideHeader) {
+  Annotations Main(R"cpp(
+#include [["a.h"]]
+void foo() {})cpp");
+  std::string MainFile = testPath("main.cc");
+  llvm::StringMap Files = {{MainFile, Main.code()},
+{testPath("a.h"), "no_type_spec;"}};
+  auto FS = buildTestFS(Files);
+
+  EXPECT_THAT(build(MainFile, Files).getDiagnostics(),
+  UnorderedElementsAre(Diag(Main.range(),
+"Error in header: C++ requires a type "
+"specifier for all declarations")));
+}
+
+TEST(DiagsInHeaders, DiagInTransitiveInclude) {
+  Annotations Main(R"cpp(
+#include [["a.h"]]
+void foo() {})cpp");
+  std::string MainFile = testPath("main.cc");
+  llvm::StringMap Files = {{MainFile, Main.code()},
+{testPath("a.h"), "#include \"b.h\""},
+{testPath("b.h"), "no_type_spec;"}};
+  auto FS = buildTestFS(Files);
+
+  EXPECT_THAT(build(MainFile, Files).getDiagnostics(),
+  UnorderedElementsAre(Diag(Main.range(),
+"Error in header: C++ requires a type "
+"specifier for all declarations")));
+}
+
+TEST(DiagsInHeaders, DiagInMultipleHeaders) {
+  Annotations Main(R"cpp(
+#include $a[["a.h"]]
+#include $b[["b.h"]]
+void foo() {})cpp");
+  std::string MainFile = testPath("main.cc");
+  llvm::StringMap Files = {{MainFile, Main.code()},
+{testPath("a.h"), "no_type_spec;"},
+{testPath("b.h"), "no_type_spec;"}};
+  auto FS = buildTestFS(Files);
+
+  EXPECT_THAT(build(MainFile, Files).getDiagnostics(),
+  UnorderedElementsAre(
+  Diag(Main.range("a"), "Error in header: C++ requires a type "
+"specifier for all declarations"),
+  Diag(Main.range("b"), "Error in header: C++ requires a type "
+"specifier for all declarations")));
+}
+
+TEST(DiagsInHeaders, PreferDirectIncludeLocation) {
+  Annotations Main(R"cpp(
+#include "a.h"
+#include [["b.h"]]
+void foo() {})cpp");
+  std::string MainFile = testPath("main.cc");
+  llvm::StringMap Files = {
+  {MainFile, Main.code()},
+  {testPath("a.h"), "#include \"b.h\"\n"},
+  {testPath("b.h"), "#ifndef X\n#define X\nno_type_spec;\n#endif"}};
+  auto FS = buildTestFS(Files);
+
+  EXPECT_THAT(build(MainFile, Files).getDiagnostics(),
+  Unordere

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

30 matches

Site Navigation

Mail list logo

Footer information