This is an archive of the discontinued LLVM Phabricator instance.

Tell whether file/folder for include completions.
ClosedPublic

Authored by kadircet on Sep 26 2018, 6:15 AM.

Diff Detail

Event Timeline

kadircet created this revision.Sep 26 2018, 6:15 AM

A drive-by comment.
Would it be cleaner to pass this information from clang? Relying on completion label seems shaky.

A drive-by comment.
Would it be cleaner to pass this information from clang? Relying on completion label seems shaky.

Actually I also wanted to do that at first, but then wasn't really sure whether we should introduce a new contextkind or not, because it is more about lsp and wasn't sure if it would be widely applicable to have a distinction between file/directory(folder) as a completion context kind.
WDYT @sammccall , would introducing a new completion context carry its weight?

sammccall accepted this revision.Sep 27 2018, 7:16 AM

A drive-by comment.
Would it be cleaner to pass this information from clang? Relying on completion label seems shaky.

Actually I also wanted to do that at first, but then wasn't really sure whether we should introduce a new contextkind or not, because it is more about lsp and wasn't sure if it would be widely applicable to have a distinction between file/directory(folder) as a completion context kind.
WDYT @sammccall , would introducing a new completion context carry its weight?

What Ilya proposes would indeed be cleaner - I think we could attach directory_entry or path/file_type to the completion result.

On the other hand while this is a hack, it's simple and it works, and the cleaner solution doesn't bring any concrete benefit at this point.

I think we should leave this be and revisit if we start querying headers from the index. (At that point we'll want more robust path info to deduplicate sema and index results).

clangd/CodeComplete.cpp
352

this probably deserves at least a comment mentioning that Sema could provide more semantic info here.

clangd/Protocol.h
706–714

This is a type outside of the core must-be-supported range, so we probably need to add the logic to ClangdLSPServer to respect the client capabilities (see adjustKindToCapability). This could be a separate patch, as it's basically plumbing.

This revision is now accepted and ready to land.Sep 27 2018, 7:16 AM
kadircet updated this revision to Diff 167320.Sep 27 2018, 7:20 AM
kadircet marked 2 inline comments as done.

Preparing the patch to respect client capabilities.

  • Add other itemkinds as well.
  • Address comments.
This revision was automatically updated to reflect the committed changes.