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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
