[PATCH] D55250: [clangd] Enhance macro hover to see full definition

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdLSPServer.cpp:816
+            // If the client supports Markdown, convert from plaintext here.
+            if (*H && HoverSupportsMarkdown) {
+              (*H)->contents.kind = MarkupKind::Markdown;
----------------
malaperle wrote:
> I don't know if you meant plain text as non-language-specific markdown or no markdown at all. In VSCode, non-markdown for multiline macros looks bad because the newlines are ignored.
Did not know about that, thanks for pointing it out.
It does not ignore double newlines though (that's what we use in declarations). I suspect it treats plaintext as markdown, but can't be sure.

In any case, converting to a markdown code block here looks incredibly hacky and shaky.
Could we use double-newlines for line breaks in our implementation instead?

This aligns with what we did before this patch for declarations.
If that doesn't work, breaking the multi-line macros in VSCode looks ok, this really feels like a bug in VSCode.


================
Comment at: clangd/Protocol.cpp:251
+        auto *A = HoverMarkupKinds->getAsArray();
+        if (!A) {
+          return false;
----------------
NIT: remove braces 


================
Comment at: clangd/Protocol.cpp:255
+
+        for (size_t I = 0; I < A->size(); ++I) {
+          MarkupKind KindOut;
----------------
NIT: use range-based-for:
```
for (auto &&E : A) {
 // ...
}
```


================
Comment at: clangd/Protocol.cpp:257
+          MarkupKind KindOut;
+          if (fromJSON((*A)[I], KindOut)) {
+            if (KindOut == MarkupKind::Markdown) {
----------------
NIT: merge ifs for better readability
``` 
if (fromJSON(...) && KindOut == Markdown) {
  // ....
}
```


================
Comment at: clangd/Protocol.cpp:638
+bool fromJSON(const llvm::json::Value &E, MarkupKind &Out) {
+  if (auto T = E.getAsString()) {
+    if (*T == "plaintext")
----------------
use [[ https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code | early exits ]] to reduce nesting.

```
auto T = E.getAsString();
if (!T)
  return false;
// rest of the code...
```



================
Comment at: clangd/XRefs.cpp:563
+  H.contents.kind = MarkupKind::PlainText;
+  H.contents.value = "#define " + Definition;
   return H;
----------------
Follow-up for the previous comment:
would it work to do `replace(Defintion, "\n", "\n\n")` here? (with a fixme comment that otherwise VSCode misbehaves)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55250