[PATCH] D71555: [clangd] Refurbish HoverInfo::present

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Jan 8 03:51:19 PST 2020


sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:393
+// Converts a string of the form `word1-word2-...` into `Word1 Word2 ...`.
+std::string beautify(llvm::StringRef Input) {
+  std::string Res;
----------------
It's worth noting that an alternative to mechanically transforming would simply be to change the getSymbolKindString(). It's only used in tests (we'd have to update the tests though).


================
Comment at: clang-tools-extra/clangd/Hover.cpp:465
   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));
+  markup::Paragraph &P = Output.addParagraph();
+  P.appendText(beautify(index::getSymbolKindString(Kind)));
----------------
Can we give P a real names like "Header"?


================
Comment at: clang-tools-extra/clangd/Hover.cpp:465
   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));
+  markup::Paragraph &P = Output.addParagraph();
+  P.appendText(beautify(index::getSymbolKindString(Kind)));
----------------
sammccall wrote:
> Can we give P a real names like "Header"?
maybe add a comment with an example, to illustrate what this section does like
// variable `x` : `int`


================
Comment at: clang-tools-extra/clangd/Hover.cpp:466
+  markup::Paragraph &P = Output.addParagraph();
+  P.appendText(beautify(index::getSymbolKindString(Kind)));
+  if (!Name.empty()) {
----------------
is beautify() actually better here? I actually quite like the lowercase as it reduces the emphasis on the kind vs the name, and the hyphens as it makes it easier to pick out the name.

This is up to you of course.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:469
+    P.appendCode(Name);
+    if (Type && !ReturnType) {
+      P.appendText(":");
----------------
if ReturnType exists and non-void, we could consider `->` or `→` in place of `:`.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:474
+  } else if (Type) {
+    P.appendCode(*Type);
   }
----------------
what *is* the case where something has a type but no name?


================
Comment at: clang-tools-extra/clangd/Hover.cpp:479
+    markup::Paragraph &P = Output.addParagraph();
+    P.appendText("Returns: ");
+    P.appendCode(*ReturnType);
----------------
consider "Returns " without the colon, or "-> " or "→"


================
Comment at: clang-tools-extra/clangd/Hover.cpp:484
+  if (Parameters && !Parameters->empty()) {
+    markup::BulletList &L = Output.addBulletList();
+    for (const auto &Param : *Parameters) {
----------------
Random aside:  If we could customize the bullet character, making it something like `←` would somewhat compensate for the lack of header here.
In plain text this would be fairly easy (need to add the API) but markdown is trickier of course - can only be done through CSS, and there's no standard way to add a CSS class.



================
Comment at: clang-tools-extra/clangd/Hover.cpp:495
+    markup::Paragraph &P = Output.addParagraph();
+    P.appendText("Value: ");
+    P.appendCode(*Value);
----------------
consider "Value = " or even just "= "


================
Comment at: clang-tools-extra/clangd/Hover.cpp:505
+    llvm::raw_string_ostream OS(Code);
+    if (NamespaceScope) {
+      // Drop trailing "::".
----------------
The assumption that unset namespace scope means no local scope etc is probably true, but seems fairly fragile/coupled.

What about 
if (LocalScope && !LocalScope->empty()) {
  // local scope
} else if (NamespaceScope && !NamespaceScope->empty())) {
  // namespace scope
} else if (NamespaceScope) {
  // global namespace
}


================
Comment at: clang-tools-extra/clangd/Hover.cpp:511
+        // function foo, class X, method X::bar, etc.
+        OS << "// In " << llvm::StringRef(LocalScope).drop_back(2);
+      } else if (!NamespaceScope->empty()) {
----------------
rtrim(':') would be clearer and more robust I think


================
Comment at: clang-tools-extra/clangd/Hover.cpp:513
+      } else if (!NamespaceScope->empty()) {
+        OS << "// namespace " << llvm::StringRef(*NamespaceScope).drop_back(2);
+      } else {
----------------
Either "// in namespace ..." or "// namespace ... {" or "namespace ... {" ?


================
Comment at: clang-tools-extra/clangd/Hover.cpp:515
+      } else {
+        OS << "// global namespace";
+      }
----------------
either "// in global namespace" or nothing at all here?

(This text is probably annoying for non-C++ projects, or C++ projects that don't use namespaces - I'd be tempted to let the absence of a NS stand for itself)


================
Comment at: clang-tools-extra/clangd/Hover.cpp:517
+      }
+      OS << '\n';
+    }
----------------
nit: seems clearer/less fragile to put this on each comment


================
Comment at: clang-tools-extra/clangd/Hover.cpp:520
+    OS << Definition;
+    Output.addCodeBlock(OS.str());
+  }
----------------
If you're going to use comment syntax, shouldn't the comment be part of the code block?


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:1442
 
+TEST(Hover, Present) {
+  struct {
----------------
This is where I really miss the render-for-tests function.

I don't think just testing plain-text is really enough, as it doesn't e.g. distinguish code from non code (such a bug exists invisibly in these tests in fact).

On the other hand, the exact markdown string generated doesn't matter as long as it has the right structure, so asserting on it is unnecessarily fragile. Might still be the right option.




================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:1442
 
+TEST(Hover, Present) {
+  struct {
----------------
sammccall wrote:
> This is where I really miss the render-for-tests function.
> 
> I don't think just testing plain-text is really enough, as it doesn't e.g. distinguish code from non code (such a bug exists invisibly in these tests in fact).
> 
> On the other hand, the exact markdown string generated doesn't matter as long as it has the right structure, so asserting on it is unnecessarily fragile. Might still be the right option.
> 
> 
Generally I think there are too many tests here, and they are too small, and so don't cover enough. I think we want to test
 - output for all the attributes (this is covered)
 - how they combine with each other (this is mostly not covered)
 - that present/absent both work

So maybe 3-4 cases:
 - a class with template parameters, documentation and definition, in global NS
 - a function with type, return type and params, in a namespace
 - a variable with type, in a function or class, with a value
should about cover it?



================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:1447
+  } Cases[] = {
+      // Basic coverage tests.
+      {
----------------
I think this comment belongs on the test function rather than inside the array


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:1450
+          [](HoverInfo &HI) { HI.Kind = index::SymbolKind::Unknown; },
+          R"pt(<unknown>)pt",
+      },
----------------
Just R"()"? The pt seems like noise.


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:1462
+            HI.Kind = index::SymbolKind::NamespaceAlias;
+            HI.Name = "foo";
+          },
----------------
what does this test incrementally to the previous test case?


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:1469
+            HI.Kind = index::SymbolKind::Class;
+            HI.Type = "type";
+          },
----------------
this example is nonsensical I think? Classes don't have a type. Same for some below.


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:1497
+          [](HoverInfo &HI) {
+            HI.Parameters.emplace();
+            HoverInfo::Param P;
----------------
This tests parameter ordering in isolation, but not e.g. how the parameter list is ordered with respect to the other sections.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71555





More information about the cfe-commits mailing list