ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.
================
Comment at: clang-tools-extra/clangd/FormattedString.cpp:41
+
+void FormattedString::appendCodeBlock(std::string Code) {
+ Chunk C;
----------------
sammccall wrote:
> you may want to take the language name, and default it to either cpp or
> nothing.
> Including it in the triple-backtick block can trigger syntax highlighting,
> and editors are probably somewhat likely to actually implement this.
Done. Defaulting to 'cpp'.
VSCode actually does syntax highlighting there and it looks nice.
================
Comment at: clang-tools-extra/clangd/FormattedString.cpp:63
+ case ChunkKind::InlineCodeBlock:
+ R += " `";
+ R += escapeBackticks(C.Contents);
----------------
sammccall wrote:
> why do we add surrounding spaces if we don't do the same for text chunks?
a leftover from my first prototype.
They don't seem to be necessary, removed them.
================
Comment at: clang-tools-extra/clangd/FormattedString.h:31
+ /// newlines.
+ void appendCodeBlock(std::string Code);
+ /// Append an inline block of C++ code. This translates to the ` block in
----------------
sammccall wrote:
> Having various methods that take strings may struggle if we want a lot of
> composability (e.g. a bullet list whose bullet whose third word is a
> hyperlink/bold).
>
> (I'm not sure whether this is going to be a real problem, just thinking out
> loud here)
>
> One alternative extensibility would be to make "containers" methods taking a
> lambda that renders the body.
> e.g.
>
> ```
> FormattedString S;
> S << "std::";
> S.bold([&} S << "swap" };
> S.list(FormattedString::Numbered, [&] {
> S.item([&] { S << "sometimes you get the wrong overload"; });
> S.item([&] {
> S << "watch out for ";
> S.link("https://en.cppreference.com/w/cpp/language/adl";, [&] { S <<
> "ADL"; });
> });
> });
> S.codeBlock([&] S << "Here is an example"; });
> ```
>
> Not sure if this is really better, I just have it on my mind because I ended
> up using it for the `JSON.h` streaming output. We can probably bolt it on
> later if necessary.
Agree that the current approach won't generalize to more complicated cases
without some design work.
The lambda-heavy code is hard to read to my personal taste, but may actually be
a good option in practice.
I'd also try playing around with some lambda-free alternatives to see if they
lead to better syntax without compromising extensibility or making the
implementation overly complex.
I also think we should bolt on this until we hit more use-cases with more
extensive needs for formatting markdown.
================
Comment at: clang-tools-extra/clangd/FormattedString.h:39
+ std::string renderAsPlainText() const;
+
+private:
----------------
sammccall wrote:
> I think we want renderForDebugging() or so which would print the chunks
> explicitly, a la CodeCompletionString.
>
> This is useful for actual debugging, and for testing methods that return
> FormattedString (as opposed to the FormattedString->markdown translation)
That would be a useful addition. I've added a FIXME for now and testing
plaintext in XRefs.
I'd suggest adding debug representation in a follow-up change, but also happy
to do it now if you feel that's necessary.
That would need updates to all hover tests and that's a big enough to go into a
separate change, from my perspective.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58547/new/
https://reviews.llvm.org/D58547
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
