This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add TemplateArgumentList into Symbol
ClosedPublic

Authored by kadircet on Mar 21 2019, 6:46 AM.

Details

Summary

Part of re-landing rC356541 with D59599. Changes the way we store
template arguments, previous patch was storing them inside Name field of Symbol.
Which was violating the assumption:

Symbol::Scope+Symbol::Name == clang::clangd::printQualifiedName

which was made in multiple places inside codebase. This patch instead moves
those arguments into their own field. Currently the field is meant to be
human-readable, can be made structured if need be.

Diff Detail

Repository
rL LLVM

Event Timeline

kadircet created this revision.Mar 21 2019, 6:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2019, 6:46 AM

should we update YAML?

clang-tools-extra/clangd/index/Symbol.h
48 ↗(On Diff #191679)

How about TemplateSpecializationArgs?

Could you also put this field near ReturnType or Signature?

clang-tools-extra/clangd/index/SymbolCollector.cpp
526 ↗(On Diff #191679)

put this near ReturnType initialization.

clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
396 ↗(On Diff #191679)

this test is getting hard to read. could you only make minimum change to the existing test and add a new case for the new behavior?

kadircet updated this revision to Diff 191860.Mar 22 2019, 4:53 AM
kadircet marked 3 inline comments as done.
  • Address comments

should we update YAML?

I suppose we are only keeping it alive for the sake of tests, but that seems like an enough reason updating that as well.

clang-tools-extra/clangd/index/SymbolCollector.cpp
526 ↗(On Diff #191679)

tried to move it to closest place I can get, can't put after this if since template specialization params are not indexed for code completion, we'll simply end up dropping those.

ilya-biryukov added inline comments.Apr 12 2019, 1:25 AM
clang-tools-extra/clangd/index/SymbolCollector.cpp
566 ↗(On Diff #191860)

Any reason to not always fill in this field?

kadircet marked an inline comment as done.Apr 12 2019, 1:39 AM
kadircet added inline comments.
clang-tools-extra/clangd/index/SymbolCollector.cpp
566 ↗(On Diff #191860)

It is simply because printTemplateSpecializationArgs should return an empty string in other cases. Since we mark all explicit specializations as *not* indexed for code completion.

ilya-biryukov added inline comments.Apr 12 2019, 2:08 AM
clang-tools-extra/clangd/index/SymbolCollector.cpp
566 ↗(On Diff #191860)

NIT: That's is not obvious. Could we fill it right after Name and Scope?
These 3 fields are related, therefore feels natural to have code filling them together.

kadircet updated this revision to Diff 194820.Apr 12 2019, 2:14 AM
kadircet marked 2 inline comments as done.
  • Fill in the TemplateSpecializationArgs in all code paths.
This revision is now accepted and ready to land.Apr 12 2019, 2:29 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2019, 3:10 AM