Page MenuHomePhabricator

[clangd] Refurbish HoverInfo::present
ClosedPublic

Authored by kadircet on Dec 16 2019, 8:52 AM.

Details

Summary

Improves basic hover presentation logic to include more info.

Diff Detail

Event Timeline

kadircet created this revision.Dec 16 2019, 8:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 16 2019, 8:52 AM
kadircet updated this revision to Diff 234087.Dec 16 2019, 8:53 AM
  • Get rid of irrelevant change
sammccall added inline comments.Jan 8 2020, 3:51 AM
clang-tools-extra/clangd/Hover.cpp
418

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).

470–485

Can we give P a real names like "Header"?

470–485

maybe add a comment with an example, to illustrate what this section does like
// variable x : int

471

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.

474

if ReturnType exists and non-void, we could consider -> or in place of :.

479

what *is* the case where something has a type but no name?

490

consider "Returns " without the colon, or "-> " or "→"

495

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.

506

consider "Value = " or even just "= "

513

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

}

519

rtrim(':') would be clearer and more robust I think

521

Either " in namespace ..." or " namespace ... {" or "namespace ... {" ?

523

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)

525

nit: seems clearer/less fragile to put this on each comment

528

If you're going to use comment syntax, shouldn't the comment be part of the code block?

clang-tools-extra/clangd/unittests/HoverTests.cpp
1522

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.

1522

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?

1527

I think this comment belongs on the test function rather than inside the array

1530

Just R"()"? The pt seems like noise.

1542

what does this test incrementally to the previous test case?

1549

this example is nonsensical I think? Classes don't have a type. Same for some below.

1577

This tests parameter ordering in isolation, but not e.g. how the parameter list is ordered with respect to the other sections.

kadircet marked 23 inline comments as done.Jan 8 2020, 8:54 AM
kadircet added inline comments.
clang-tools-extra/clangd/Hover.cpp
418

or we can have our own SymbolKind to string conversion. I would be OK with doing that(rather than changing getSymbolKindString) if you believe mechanical conversion isn't enough.

471

I've actually found those hyphens annoying, but don't have any strong feelings towards those. I would land it this way and see how ppl feel about it. (but of course, if you've got any strong feelings we could also do it the other way around)

474

we've got a special Returns: x paragraph for function-likes, therefore I wasn't printing anything after the name for functions.

but thinking about it now, actually it makes sense to drop that line and print something like:

Function `foo` -> `int`
- `bool param1`

wdyt?

479

there's none, it was a leftover from old revisions, nvm.

521

going with In namespace ..., having a { in the end without an enclosing } doesn't seem right.

523

agreed, dropped that case and put some comments to explain why.

528

it is, I am putting everything to stream and then creating a codeblock out of it.

clang-tools-extra/clangd/unittests/HoverTests.cpp
1542

it was rather for checking replacement of hyphens.

kadircet updated this revision to Diff 236842.Jan 8 2020, 8:54 AM
kadircet marked 5 inline comments as done.
  • Address comments

Unit tests: pass. 61307 tests passed, 0 failed and 736 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

sammccall accepted this revision.Jan 8 2020, 9:30 AM

Great stuff, let's finally ship it!

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

ok, that's fine. If you don't care much about the case, consider dropping the case transform and just replacing - with , it seems less noisy. But up to you, fine to leave as-is.

474

Yeah, I think that'd be better and more consistent with Type.
(except in cases where the name and/or return type are long, but that's probably equally true with Type/ReturnType)

475

if there's no name, do we want to skip the header entirely?
(I can't think of examples for this case.)

528

Right, sorry!

nit: OS here seems a little obscure/overkill, could be just

std::string ScopeComment;
if (...) {
  ScopeComment = "//...\n";
}
Output.addCodeBlock(ScopeComment + Definition);
clang-tools-extra/clangd/unittests/HoverTests.cpp
1542

right - maybe delete the "class foo" test then?

1630

I do find the uppercase A next to the lowercase foo here jarring.

This looks like "title case", but in title case the filler words are lowercase and the "important" ones are capitalized, and here we'd have the opposite.

"Namespace alias foo" or "namespace alias foo" both seem fine.

This revision is now accepted and ready to land.Jan 8 2020, 9:30 AM
kadircet updated this revision to Diff 236976.Jan 9 2020, 1:05 AM
kadircet marked 10 inline comments as done.

Address comments

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

got rid of beautify, printing getSymbolKindString as is.

475

HoverInfo::Name is always filled currently, as we always get a NamedDecl(after change to explicitReferenceTargets).

Changing it accordingly.

506

sorry somehow missed that one.

what about putting that into header part with something like:

variable `var` : `int`(=3)

function `foo` -> `int`(=3)

we can also drop equals signs, I am not sure if it looks confusing without those.

Unit tests: pass. 61307 tests passed, 0 failed and 736 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

sammccall accepted this revision.Jan 9 2020, 1:31 AM

still LG, go ahead and we can iterate

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

I'm not sure if this is a good idea as all of name, type, and value can be long. If we overrun and end up wrapping, I think it's less clear than a separate paragraph.

That said if you do go this way, I'd vote to keep the = and drop the (), I think that's clear and terse

kadircet updated this revision to Diff 236993.Jan 9 2020, 2:25 AM
  • Change Value: to Value =
kadircet marked 2 inline comments as done.Jan 9 2020, 2:25 AM
This revision was automatically updated to reflect the committed changes.

Unit tests: pass. 61307 tests passed, 0 failed and 736 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml