Page MenuHomePhabricator

[clangd] Add a ruler after header in hover
ClosedPublic

Authored by kadircet on Jan 13 2020, 8:18 AM.

Diff Detail

Event Timeline

kadircet created this revision.Jan 13 2020, 8:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2020, 8:18 AM

Unit tests: unknown.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt

sammccall accepted this revision.Jan 13 2020, 10:35 AM
sammccall added inline comments.
clang-tools-extra/clangd/Hover.cpp
535

(When merging with D72623, I'm assuming return type will go below this line?)

558

seems to me we'd likely want one above definition too

clang-tools-extra/clangd/test/hover.test
12 ↗(On Diff #237687)

Why are we getting two blank lines here?

(I understand why this patch increases it by one, but don't understand why it was two before. Ideally I think we never want \n\n\n in plaintext)

This revision is now accepted and ready to land.Jan 13 2020, 10:35 AM
kadircet updated this revision to Diff 237886.Jan 14 2020, 1:59 AM
kadircet marked 5 inline comments as done.
  • Increase control around paddings for plaintext rendering.
  • Add another ruler before definition.
kadircet updated this revision to Diff 237887.Jan 14 2020, 1:59 AM
  • Update comments
kadircet added inline comments.Jan 14 2020, 2:05 AM
clang-tools-extra/clangd/Hover.cpp
535

yes it will

clang-tools-extra/clangd/test/hover.test
12 ↗(On Diff #237687)

it was two before because of extra padding around codeblocks.

but agreed that we shouldn't have that much spacing.

Unit tests: pass. 61802 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

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

Unit tests: pass. 61802 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

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

Harbormaster completed remote builds in B43911: Diff 237887.
kadircet updated this revision to Diff 237934.Jan 14 2020, 4:15 AM
  • Per offline discussions, do post processing to get rid of multiple ruler occurences.

Unit tests: pass. 61819 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

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

sammccall added inline comments.Jan 14 2020, 10:07 AM
clang-tools-extra/clangd/FormattedString.cpp
118

Having thought about this a bit more:

I think it'd be clearer to apply the ruler-filtering "symbolically", dropping those blocks before attempting rendering. Need isRuler() I guess.
Then something like

vector<Block*> FilteredBlocks;
for (auto& C : Children) {
  if (C->isRuler() && (FilteredBlocks.empty() || FilteredBlocks.back()->isRuler()))
    continue;
  FilteredBlocks.push_back(C.get());
}
while (!FilteredBlocks.empty() && FilteredBlocks.back()->isRuler())
  FilteredBlocks.pop_back();

Whereas I can't see a good way to apply the plaintext-newline rule symbolically, textually seems reasonable:

llvm::erase_if(Text, [&](Char &C) { return StringRef(Text.data(), &C - Text.data()).endswith("\n\n"); });
clang-tools-extra/clangd/FormattedString.h
36

This is a bit weird layering-wise. I think it'd be cleaner just to do a substitution on the rendered output.
Still a hack, but we've got that either way.

kadircet updated this revision to Diff 238243.Jan 15 2020, 6:49 AM
kadircet marked 2 inline comments as done.
  • Address comments
This revision was automatically updated to reflect the committed changes.

Unit tests: pass. 61885 tests passed, 0 failed and 782 were skipped.

clang-tidy: unknown.

clang-format: pass.

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