Page MenuHomePhabricator

[clangd] Fix handling of inline/anon namespaces and names of deduced types in hover
ClosedPublic

Authored by kadircet on Dec 16 2019, 5:43 AM.

Details

Summary

Clangd normally skips inline and anon namespaces while printing nested name
specifiers. It also drops any tag specifiers since we make use of HoverInfo::Kind
instead of some text in HoverInfo::Name

There was a bug causing us to print innermost inline/anon namespace, this patch
fixes that by skipping those.
Also changes printing and kind detection of deduced types to be similar to decl
case.

Diff Detail

Event Timeline

kadircet created this revision.Dec 16 2019, 5:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 16 2019, 5:43 AM
kadircet retitled this revision from [clangd] Fix handling of inline and anon namespaces in hover to [clangd] Fix handling of inline/anon namespaces and names of deduced types in hover.Dec 16 2019, 5:47 AM
kadircet edited the summary of this revision. (Show Details)
kadircet added a reviewer: ilya-biryukov.

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

clang-format: pass.

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

My biggest concern is that we seem to make output for template instantiation worse.
There should be a way to stop showing anonymous namespace without introducing such regressions.

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

Not related to this patch, but what is D here? Is this getting hover contents for a type or for a decl?

358–359

NIT: could be simplified to

if (!D)
  D = T->getAsTagDecl();
if (!D)
  return HI;

// ... body of FillInHover goes here
clang-tools-extra/clangd/unittests/HoverTests.cpp
365

NIT: could you give an example how you want the output to look like?

376

Foo<int> actually looked better. Do you consider this a regression or is this intended?

388

Why does this give different output from the previous example?
I would argue they should both be consistent. Users shouldn't care if there's an explicit specialization or not.

532

What if anon/inline namespace are interleaves with named ones?
What would it print?

namespace a { inline namespace inl {  namespace b { namespace { namespace c { namespace {
struct X {};
}}}}}}

Could we test this?

1214

This looks like a regression. What's stopping us from fixing this right away?

kadircet updated this revision to Diff 234050.Dec 16 2019, 6:43 AM
kadircet marked 9 inline comments as done.
  • Address comments

My biggest concern is that we seem to make output for template instantiation worse.
There should be a way to stop showing anonymous namespace without introducing such regressions.

I've got D71545 to reduce that regression.

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

it represents the deduced decl for Type, if any.

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

See D71544

376

See D71545

388

i totally agree. this one has a different output because of explicit specializations having a template pattern.
this is a temporary regression that should be fixed by D71545 (i am planning to land those patches as a whole)

Unit tests: fail. 60930 tests passed, 2 failed and 726 were skipped.

failed: libc++.std/thread/thread_mutex/thread_mutex_requirements/thread_mutex_requirements_mutex/thread_mutex_recursive/lock.pass.cpp
failed: libc++.std/thread/thread_mutex/thread_mutex_requirements/thread_mutex_requirements_mutex/thread_mutex_recursive/try_lock.pass.cpp

clang-format: pass.

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

I've got D71545 to reduce that regression.

D71545 seems to be pretty small, yet depends on this change. Maybe add it right here?
Otherwise clangd is in a kinda broken state between those two commits, not a very good state to be at.
Would also make it simpler to revert the change in case something bad happens.

ilya-biryukov added inline comments.Dec 17 2019, 2:12 AM
clang-tools-extra/clangd/Hover.cpp
354

What is a "deduced decl for Type"?

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

D71544 is trivial, could you put it into this patch?
Having a FIXME that is fixed in the follow-up with a one-line change seems to potentially complicate things (reverts, reading change history).

kadircet marked 3 inline comments as done.Dec 17 2019, 2:34 AM
kadircet added inline comments.
clang-tools-extra/clangd/Hover.cpp
354

it was referring to the tagdecl referred by decltypes, i am not sure if there are cases in which this can be different than T->getAsTagDecl.
Always making use of T->getAsTagDecl doesn't seem to be causing any test failures.

kadircet updated this revision to Diff 234249.Dec 17 2019, 2:34 AM
  • Squash follow-ups
kadircet updated this revision to Diff 234250.Dec 17 2019, 2:36 AM
  • Delete fixme

Unit tests: pass. 60932 tests passed, 0 failed and 726 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

Unit tests: pass. 60932 tests passed, 0 failed and 726 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

ilya-biryukov added inline comments.Dec 17 2019, 3:35 AM
clang-tools-extra/clangd/Hover.cpp
354

if T->getAsTagDecl() is non-null, why does D being passed to the function is null?
How can they be different?

ilya-biryukov added inline comments.Dec 17 2019, 3:40 AM
clang-tools-extra/clangd/Hover.cpp
369

NIT: is flush redundant? I believe it's called in destructor

423

Is this the only callsite of getHoverContents that we are changin?
We could just move the logic that computes D into the getHoverContents.

kadircet updated this revision to Diff 234287.Dec 17 2019, 6:34 AM
kadircet marked 6 inline comments as done.
  • Address comments
  • Get rid of deduced decl in decltype/auto case
clang-tools-extra/clangd/Hover.cpp
354

see D71597

This revision is now accepted and ready to land.Dec 17 2019, 7:06 AM

Unit tests: fail. 60977 tests passed, 1 failed and 727 were skipped.

failed: lit.lit/shtest-format.py

clang-tidy: unknown.

clang-format: unknown.

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

Unit tests: fail. 60978 tests passed, 1 failed and 727 were skipped.

failed: lit.lit/shtest-format.py

clang-tidy: unknown.

clang-format: unknown.

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

This revision was automatically updated to reflect the committed changes.