This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Introduce paragraph, the first part of new rendering structs
ClosedPublic

Authored by kadircet on Dec 10 2019, 3:21 AM.

Details

Summary

Initial patch for new rendering structs in clangd.

Splitting implementation into smaller chunks, for a full view of the API see D71063.

Diff Detail

Event Timeline

kadircet created this revision.Dec 10 2019, 3:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 10 2019, 3:21 AM

Build result: pass - 60655 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

kadircet updated this revision to Diff 233078.Dec 10 2019, 6:26 AM
  • More comments

Build result: pass - 60655 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

Main thing here is I'm fairly sure that code blocks shouldn't be chunks in a paragraph (or I'm misunderstanding what a paragraph is)

clang-tools-extra/clangd/FormattedString.cpp
127

is this equivalent to

SmallVector<StringRef> Words;
llvm::SplitString(Input, Words);
return llvm::join(Input, " ");

?

(That would trim leading/trailing whitespace, but I suspect we actually want that)

168–169

Can we simplify the whitespace model? A lot of this is hacks around code blocks, which shouldn't really be here.

What about:

  • the contents of each chunk never have leading/trailing ws (it's stripped when adding)
  • there's always a space between consecutive chunks
clang-tools-extra/clangd/FormattedString.h
24–25

I don't think this comment captures what this class is about now. (Rewriting the class but leaving the comment intact is suspicious!)

Comment should allude to how blocks make up a document - a RenderableBlock holds rich text and knows how to lay it out. Blocks can be stacked to form a document, etc.

25–28

nit: I wouldn't bother with both "renderable" prefix and the markup namespace, up to you

30

Or just asMarkdown, up to you

30

are you sure we want to create all the temporary strings?
Consider void renderMarkdown(llvm::raw_ostream&), and keep std::string asMarkdown() on Document

32

renderfortests has gone. What's the plan for testing Hover rendering? (i.e. HoverInfo -> Document)
I guess asserting the plain text and markdown might be enough...

(Did we remove *all* the tests for present()in D70911, and I didn't notice? We should restore some based on synthetic HoverInfo)

37

A code block doesn't go in a paragraph, as it's a block itself.

"A paragraph contains a logical line of rich text. It may be wrapped for display"

46–47

nit: while here, I do think inline block is misleading - this corresponds to display: inline, not display:inline-block. "inline span"?

46–47

just appendCode?

65

semantic -> format-agnostic? This doesn't capture semantics.

formattable strings -> rich text, or structured text?

kadircet updated this revision to Diff 233420.Dec 11 2019, 11:11 AM
kadircet marked 12 inline comments as done.
  • Move code blocks out of Paragraph
  • Provide a way to control vertical spacing
  • Address comments around naming
kadircet added inline comments.Dec 11 2019, 11:12 AM
clang-tools-extra/clangd/FormattedString.cpp
127

yes that's exactly what we want, thanks!
not just joining them though, since we could still merge them in-place.

clang-tools-extra/clangd/FormattedString.h
32

yes we I was planning to check using the plaintext output with some "synthetic" hoverinfo. Currently only lit tests are checking for the behavior of present.
there's also changes to hover.cpp in this patch to preserve current output. happy to add some tests before moving forward, but didn't want to add any as they
will change drastically with the new rendering layout.

37

sending out codeblocks in a new patch

Unit tests: pass. 60655 tests passed, 0 failed and 726 were skipped.

clang-format: pass.

Build artifacts: console-log.txt, CMakeCache.txt, test-results.xml, diff.json

kadircet updated this revision to Diff 233513.Dec 11 2019, 11:32 PM
  • More tests

Unit tests: pass. 60657 tests passed, 0 failed and 726 were skipped.

clang-format: pass.

Build artifacts: console-log.txt, CMakeCache.txt, test-results.xml, diff.json

Design question around newlines :-) Otherwise looks good.

clang-tools-extra/clangd/FormattedString.cpp
127

I'm really not sure saving the allocation is worth the code here, but up to you.

136

(why not just pass in the fptr?)

140

I'm not sure this common codepath for plain-text and markdown will survive in the long run:

  • state around indentation and wrapping is likely to differ
  • separating blocks with one newline may not be the right thing in each case
141

for markdown, this means consecutive paragraphs will be rendered as a single paragraph with line breaks in it. intended?

148

this deserves a comment - the effect is produced by the surrounding newlines.
But if we change the blocks to include their whitespace it'll be more obvious.

clang-tools-extra/clangd/FormattedString.h
30

Do these include trailing newline(s), or not?

Based on offline discussion, they currently do not, but this causes some problems e.g. list followed by paragraph would get merged.

Option a) have Blocks include required trailing newlines (e.g. \n\n after lists in markdown, \n after paragraphs), and have Document trim trailing newlines.
Option b) have Document look at the type of the elements above/below and insert space appropriately.

Personally prefer option A because it encapsulates behavior better in the classes (less dyn_cast) and I think reflects the grammar better.

clang-tools-extra/clangd/Hover.cpp
461

This is a regression at the moment - complicated definitions are clang-formatted, and we'll canonicalize the whitespace.
(The behavior of the library is correct, we just need code block for this).

kadircet marked 12 inline comments as done.Dec 12 2019, 3:40 AM
kadircet added inline comments.
clang-tools-extra/clangd/FormattedString.cpp
140

I suppose this will survive in the long run now, as it only prints blocks, without separation, and trims in the end.

141

yes this was intended, but as each block takes care of its own indentation now, this is no longer an issue.

clang-tools-extra/clangd/FormattedString.h
30

taking option A

clang-tools-extra/clangd/Hover.cpp
461

not planning to land until we've got the complete implementation, including lists and codeblocks.

kadircet updated this revision to Diff 233566.Dec 12 2019, 3:40 AM
kadircet marked 4 inline comments as done.
  • Move separation logic from container to blocks
  • Add two concrete APIs to block for getting strings and update tests to use those.
  • Rename methods

Unit tests: fail. 60762 tests passed, 1 failed and 726 were skipped.

failed: Clang.CodeGenCXX/runtime-dllstorage.cpp

clang-format: pass.

Build artifacts: console-log.txt, CMakeCache.txt, test-results.xml, diff.json

sammccall accepted this revision.Dec 12 2019, 3:58 AM
sammccall added inline comments.
clang-tools-extra/clangd/FormattedString.cpp
164–166

this is worth a comment - we translate Paragraphs into markdown lines, not markdown paragraphs

201

push_back (and below)

clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
19–20

dead?

83

I'd consider writing this as
[&] { Para.appendText("after"); } to make it clearer what's being tested.

Are you sure the table test is clearer than straight-line code incrementally building the paragrap/asserting here?

This revision is now accepted and ready to land.Dec 12 2019, 3:58 AM
kadircet updated this revision to Diff 233576.Dec 12 2019, 4:13 AM
kadircet marked 5 inline comments as done.
  • Address comments
clang-tools-extra/clangd/unittests/FormattedStringTests.cpp
83

this test was huge in the beginning and having a straight line of appends were making it really hard to reason with. but it seems leaner now, as it doesn't have codeblocks anymore.
switching back to linear testing.

Unit tests: pass. 60763 tests passed, 0 failed and 726 were skipped.

clang-format: pass.

Build artifacts: console-log.txt, CMakeCache.txt, test-results.xml, diff.json

This revision was automatically updated to reflect the committed changes.
aganea added a subscriber: aganea.Dec 19 2019, 11:48 AM
aganea added inline comments.
clang-tools-extra/clangd/FormattedString.cpp
110

There's an issue at this line, the iterator might go past the end of the string (if running the Document.Separators test). This fails when running a Debug build with VS 2019. The code fails with this assert (in xstring):

_STL_VERIFY(_Unfancy(_Ptr) < _Mycont->_Myptr() + _Mycont->_Mysize, "cannot increment string iterator past end");

I changed the code such as:


(sorry, Phabricator throws an exception when uploading this patch as a differential)

I can commit it if you're fine with the change.

kadircet marked an inline comment as done.Dec 19 2019, 12:51 PM
kadircet added inline comments.
clang-tools-extra/clangd/FormattedString.cpp
110

thanks for reporting this!

sent out rG3346cecd4c0c960377b441606b6382a684daf061