Merge pull request #1002

LLVM pull down (356b335)

Author: vladimirlaz

clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp

@@ -86,15 +86,17 @@ MagicNumbersCheck::MagicNumbersCheck(StringRef Name, ClangTidyContext *Context)
     IgnoredDoublePointValues.reserve(IgnoredFloatingPointValuesInput.size());
     for (const auto &InputValue : IgnoredFloatingPointValuesInput) {
       llvm::APFloat FloatValue(llvm::APFloat::IEEEsingle());
-      if (!FloatValue.convertFromString(InputValue, DefaultRoundingMode)) {
-        assert(false && "Invalid floating point representation");
-      }
+      auto StatusOrErr =
+          FloatValue.convertFromString(InputValue, DefaultRoundingMode);
+      assert(StatusOrErr && "Invalid floating point representation");
+      consumeError(StatusOrErr.takeError());
       IgnoredFloatingPointValues.push_back(FloatValue.convertToFloat());
 
       llvm::APFloat DoubleValue(llvm::APFloat::IEEEdouble());
-      if (!DoubleValue.convertFromString(InputValue, DefaultRoundingMode)) {
-        assert(false && "Invalid floating point representation");
-      }
+      StatusOrErr =
+          DoubleValue.convertFromString(InputValue, DefaultRoundingMode);
+      assert(StatusOrErr && "Invalid floating point representation");
+      consumeError(StatusOrErr.takeError());
       IgnoredDoublePointValues.push_back(DoubleValue.convertToDouble());
     }
     llvm::sort(IgnoredFloatingPointValues.begin(),

clang-tools-extra/clangd/CompileCommands.cpp

@@ -155,7 +155,9 @@ void CommandMangler::adjust(std::vector<std::string> &Cmd) const {
   if (ResourceDir && !Has("-resource-dir"))
     Cmd.push_back(("-resource-dir=" + *ResourceDir));
 
-  if (Sysroot && !Has("-isysroot")) {
+  // Don't set `-isysroot` if it is already set or if `--sysroot` is set.
+  // `--sysroot` is a superset of the `-isysroot` argument.
+  if (Sysroot && !Has("-isysroot") && !Has("--sysroot")) {
     Cmd.push_back("-isysroot");
     Cmd.push_back(*Sysroot);
   }

clang-tools-extra/clangd/Diagnostics.cpp

@@ -22,6 +22,8 @@
 #include "clang/Lex/Token.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
@@ -328,14 +330,22 @@ CodeAction toCodeAction(const Fix &F, const URIForFile &File) {
 void toLSPDiags(
     const Diag &D, const URIForFile &File, const ClangdDiagnosticOptions &Opts,
     llvm::function_ref<void(clangd::Diagnostic, llvm::ArrayRef<Fix>)> OutFn) {
-  auto FillBasicFields = [](const DiagBase &D) -> clangd::Diagnostic {
-    clangd::Diagnostic Res;
-    Res.range = D.Range;
-    Res.severity = getSeverity(D.Severity);
-    return Res;
-  };
+  clangd::Diagnostic Main;
+  Main.severity = getSeverity(D.Severity);
+
+  // Main diagnostic should always refer to a range inside main file. If a
+  // diagnostic made it so for, it means either itself or one of its notes is
+  // inside main file.
+  if (D.InsideMainFile) {
+    Main.range = D.Range;
+  } else {
+    auto It =
+        llvm::find_if(D.Notes, [](const Note &N) { return N.InsideMainFile; });
+    assert(It != D.Notes.end() &&
+           "neither the main diagnostic nor notes are inside main file");
+    Main.range = It->Range;
+  }
 
-  clangd::Diagnostic Main = FillBasicFields(D);
   Main.code = D.Name;
   switch (D.Source) {
   case Diag::Clang:
@@ -379,7 +389,9 @@ void toLSPDiags(
     for (auto &Note : D.Notes) {
       if (!Note.InsideMainFile)
         continue;
-      clangd::Diagnostic Res = FillBasicFields(Note);
+      clangd::Diagnostic Res;
+      Res.severity = getSeverity(Note.Severity);
+      Res.range = Note.Range;
       Res.message = noteMessage(D, Note, Opts);
       OutFn(std::move(Res), llvm::ArrayRef<Fix>());
     }

clang-tools-extra/clangd/Hover.cpp

@@ -25,9 +25,11 @@
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/Index/IndexSymbol.h"
 #include "llvm/ADT/STLExtras.h"
-#include "llvm/ADT/iterator_range.h"
-#include "llvm/Support/Casting.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/raw_ostream.h"
+#include <string>
 
 namespace clang {
 namespace clangd {
@@ -224,8 +226,8 @@ void enhanceFromIndex(HoverInfo &Hover, const NamedDecl &ND,
 
 // Populates Type, ReturnType, and Parameters for function-like decls.
 void fillFunctionTypeAndParams(HoverInfo &HI, const Decl *D,
-                                      const FunctionDecl *FD,
-                                      const PrintingPolicy &Policy) {
+                               const FunctionDecl *FD,
+                               const PrintingPolicy &Policy) {
   HI.Parameters.emplace();
   for (const ParmVarDecl *PVD : FD->parameters()) {
     HI.Parameters->emplace_back();
@@ -250,11 +252,11 @@ void fillFunctionTypeAndParams(HoverInfo &HI, const Decl *D,
     }
   }
 
-  if (const auto* CCD = llvm::dyn_cast<CXXConstructorDecl>(FD)) {
+  if (const auto *CCD = llvm::dyn_cast<CXXConstructorDecl>(FD)) {
     // Constructor's "return type" is the class type.
     HI.ReturnType = declaredType(CCD->getParent()).getAsString(Policy);
     // Don't provide any type for the constructor itself.
-  } else if (llvm::isa<CXXDestructorDecl>(FD)){
+  } else if (llvm::isa<CXXDestructorDecl>(FD)) {
     HI.ReturnType = "void";
   } else {
     HI.ReturnType = FD->getReturnType().getAsString(Policy);
@@ -309,7 +311,7 @@ llvm::Optional<std::string> printExprValue(const SelectionTree::Node *N,
 }
 
 /// Generate a \p Hover object given the declaration \p D.
-HoverInfo getHoverContents(const Decl *D, const SymbolIndex *Index) {
+HoverInfo getHoverContents(const NamedDecl *D, const SymbolIndex *Index) {
   HoverInfo HI;
   const ASTContext &Ctx = D->getASTContext();
 
@@ -321,12 +323,10 @@ HoverInfo getHoverContents(const Decl *D, const SymbolIndex *Index) {
     HI.LocalScope.append("::");
 
   PrintingPolicy Policy = printingPolicyForDecls(Ctx.getPrintingPolicy());
-  if (const NamedDecl *ND = llvm::dyn_cast<NamedDecl>(D)) {
-    HI.Name = printName(Ctx, *ND);
-    ND = getDeclForComment(ND);
-    HI.Documentation = getDeclComment(Ctx, *ND);
-    enhanceFromIndex(HI, *ND, Index);
-  }
+  HI.Name = printName(Ctx, *D);
+  const auto *CommentD = getDeclForComment(D);
+  HI.Documentation = getDeclComment(Ctx, *CommentD);
+  enhanceFromIndex(HI, *CommentD, Index);
 
   HI.Kind = index::getSymbolInfo(D).Kind;
 
@@ -460,34 +460,70 @@ llvm::Optional<HoverInfo> getHover(ParsedAST &AST, Position Pos,
           tooling::applyAllReplacements(HI->Definition, Replacements))
     HI->Definition = *Formatted;
 
-  HI->SymRange = getTokenRange(AST.getSourceManager(),
-                               AST.getLangOpts(), SourceLocationBeg);
+  HI->SymRange = getTokenRange(AST.getSourceManager(), AST.getLangOpts(),
+                               SourceLocationBeg);
   return HI;
 }
 
 markup::Document HoverInfo::present() const {
   markup::Document Output;
-  if (NamespaceScope) {
-    auto &P = Output.addParagraph();
-    P.appendText("Declared in");
-    // Drop trailing "::".
-    if (!LocalScope.empty())
-      P.appendCode(llvm::StringRef(LocalScope).drop_back(2));
-    else if (NamespaceScope->empty())
-      P.appendCode("global namespace");
-    else
-      P.appendCode(llvm::StringRef(*NamespaceScope).drop_back(2));
+  // Header contains a text of the form:
+  // variable `var` : `int`
+  //
+  // class `X`
+  //
+  // function `foo` → `int`
+  markup::Paragraph &Header = Output.addParagraph();
+  Header.appendText(index::getSymbolKindString(Kind));
+  assert(!Name.empty() && "hover triggered on a nameless symbol");
+  Header.appendCode(Name);
+  if (ReturnType) {
+    Header.appendText("→");
+    Header.appendCode(*ReturnType);
+  } else if (Type) {
+    Header.appendText(":");
+    Header.appendCode(*Type);
   }
 
-  if (!Definition.empty()) {
-    Output.addCodeBlock(Definition);
-  } else {
-    // Builtin types
-    Output.addCodeBlock(Name);
+  // For functions we display signature in a list form, e.g.:
+  // - `bool param1`
+  // - `int param2 = 5`
+  if (Parameters && !Parameters->empty()) {
+    markup::BulletList &L = Output.addBulletList();
+    for (const auto &Param : *Parameters) {
+      std::string Buffer;
+      llvm::raw_string_ostream OS(Buffer);
+      OS << Param;
+      L.addItem().addParagraph().appendCode(std::move(OS.str()));
+    }
+  }
+
+  if (Value) {
+    markup::Paragraph &P = Output.addParagraph();
+    P.appendText("Value =");
+    P.appendCode(*Value);
   }
 
   if (!Documentation.empty())
     Output.addParagraph().appendText(Documentation);
+
+  if (!Definition.empty()) {
+    std::string ScopeComment;
+    // Drop trailing "::".
+    if (!LocalScope.empty()) {
+      // Container name, e.g. class, method, function.
+      // We might want to propogate some info about container type to print
+      // function foo, class X, method X::bar, etc.
+      ScopeComment =
+          "// In " + llvm::StringRef(LocalScope).rtrim(':').str() + '\n';
+    } else if (NamespaceScope && !NamespaceScope->empty()) {
+      ScopeComment = "// In namespace " +
+                     llvm::StringRef(*NamespaceScope).rtrim(':').str() + '\n';
+    }
+    // Note that we don't print anything for global namespace, to not annoy
+    // non-c++ projects or projects that are not making use of namespaces.
+    Output.addCodeBlock(ScopeComment + Definition);
+  }
   return Output;
 }
 

clang-tools-extra/clangd/test/hover.test

@@ -9,7 +9,7 @@
 # CHECK-NEXT:  "result": {
 # CHECK-NEXT:    "contents": {
 # CHECK-NEXT:      "kind": "plaintext",
-# CHECK-NEXT:      "value": "Declared in global namespace\n\nvoid foo()"
+# CHECK-NEXT:      "value": "function foo → void\n\nvoid foo()"
 # CHECK-NEXT:    },
 # CHECK-NEXT:    "range": {
 # CHECK-NEXT:      "end": {
@@ -37,7 +37,7 @@
 # CHECK-NEXT:  "result": {
 # CHECK-NEXT:    "contents": {
 # CHECK-NEXT:      "kind": "plaintext",
-# CHECK-NEXT:      "value": "Declared in global namespace\n\nenum foo {}"
+# CHECK-NEXT:      "value": "enum foo\n\nenum foo {}"
 # CHECK-NEXT:    },
 # CHECK-NEXT:    "range": {
 # CHECK-NEXT:      "end": {

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp

@@ -1014,6 +1014,29 @@ TEST(IgnoreDiags, FromNonWrittenInclude) {
   EXPECT_THAT(TU.build().getDiagnostics(), UnorderedElementsAre());
 }
 
+TEST(ToLSPDiag, RangeIsInMain) {
+  ClangdDiagnosticOptions Opts;
+  clangd::Diag D;
+  D.Range = {pos(1, 2), pos(3, 4)};
+  D.Notes.emplace_back();
+  Note &N = D.Notes.back();
+  N.Range = {pos(2, 3), pos(3, 4)};
+
+  D.InsideMainFile = true;
+  N.InsideMainFile = false;
+  toLSPDiags(D, {}, Opts,
+             [&](clangd::Diagnostic LSPDiag, ArrayRef<clangd::Fix>) {
+               EXPECT_EQ(LSPDiag.range, D.Range);
+             });
+
+  D.InsideMainFile = false;
+  N.InsideMainFile = true;
+  toLSPDiags(D, {}, Opts,
+             [&](clangd::Diagnostic LSPDiag, ArrayRef<clangd::Fix>) {
+               EXPECT_EQ(LSPDiag.range, N.Range);
+             });
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/unittests/HoverTests.cpp

@@ -1606,6 +1606,95 @@ TEST(Hover, DocsFromMostSpecial) {
     }
   }
 }
+TEST(Hover, Present) {
+  struct {
+    const std::function<void(HoverInfo &)> Builder;
+    llvm::StringRef ExpectedRender;
+  } Cases[] = {
+      {
+          [](HoverInfo &HI) {
+            HI.Kind = index::SymbolKind::Unknown;
+            HI.Name = "X";
+          },
+          R"(<unknown> X)",
+      },
+      {
+          [](HoverInfo &HI) {
+            HI.Kind = index::SymbolKind::NamespaceAlias;
+            HI.Name = "foo";
+          },
+          R"(namespace-alias foo)",
+      },
+      {
+          [](HoverInfo &HI) {
+            HI.Kind = index::SymbolKind::Class;
+            HI.TemplateParameters = {
+                {std::string("typename"), std::string("T"), llvm::None},
+                {std::string("typename"), std::string("C"),
+                 std::string("bool")},
+            };
+            HI.Documentation = "documentation";
+            HI.Definition =
+                "template <typename T, typename C = bool> class Foo {}";
+            HI.Name = "foo";
+            HI.NamespaceScope.emplace();
+          },
+          R"(class foo
+documentation
+
+template <typename T, typename C = bool> class Foo {})",
+      },
+      {
+          [](HoverInfo &HI) {
+            HI.Kind = index::SymbolKind::Function;
+            HI.Name = "foo";
+            HI.Type = "type";
+            HI.ReturnType = "ret_type";
+            HI.Parameters.emplace();
+            HoverInfo::Param P;
+            HI.Parameters->push_back(P);
+            P.Type = "type";
+            HI.Parameters->push_back(P);
+            P.Name = "foo";
+            HI.Parameters->push_back(P);
+            P.Default = "default";
+            HI.Parameters->push_back(P);
+            HI.NamespaceScope = "ns::";
+            HI.Definition = "ret_type foo(params) {}";
+          },
+          R"(function foo → ret_type
+- 
+- type
+- type foo
+- type foo = default
+
+// In namespace ns
+ret_type foo(params) {})",
+      },
+      {
+          [](HoverInfo &HI) {
+            HI.Kind = index::SymbolKind::Variable;
+            HI.LocalScope = "test::bar::";
+            HI.Value = "value";
+            HI.Name = "foo";
+            HI.Type = "type";
+            HI.Definition = "def";
+          },
+          R"(variable foo : type
+Value = value
+
+// In test::bar
+def)",
+      },
+  };
+
+  for (const auto &C : Cases) {
+    HoverInfo HI;
+    C.Builder(HI);
+    EXPECT_EQ(HI.present().asPlainText(), C.ExpectedRender);
+  }
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang