This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add "Deprecated" field to Symbol and CodeCompletion.
ClosedPublic

Authored by ioeric on Sep 6 2018, 1:50 AM.

Diff Detail

Event Timeline

ioeric created this revision.Sep 6 2018, 1:50 AM

I think you also need to update SymbolsYAML and Serialization.

clangd/Protocol.cpp
520

do we actually want this in JSON?
(genuinely unsure - any clients aware of this extension?)

clangd/Protocol.h
771

this is a clangd extension.
(right?)

clangd/index/Index.h
246

would you mind packing this together with IsIndexedForCompletion, for memory size?
either as an actual bitfield bool Deprecated : 1 = false or as enum flags enum Flags : uint8_t { IndexedForCompletion, Deprecated, }; Flags flags

The latter will simplify life for serialization, but up to you.

ioeric updated this revision to Diff 164205.Sep 6 2018, 6:45 AM
ioeric marked an inline comment as done.
  • Pack flags in Symbol.
clangd/Protocol.cpp
520
clangd/index/Index.h
246

Done. Pack them into flags.

kadircet added inline comments.Sep 6 2018, 9:33 AM
clangd/index/Index.h
171

nit: rename to IndexedForCodeCompletion, since it is more of an attribute having is in the name doesn't seem so cool. Instead of the attributes themselves maybe the checkers below should have "is" prefix.

172

nit: Add a comment similar to above one leading to Deprecated()?

245

nit: rename to isDeprecated ?

unittests/clangd/CodeCompleteTests.cpp
1365

Maybe do this on its own render so that we can have both code paths covered. Just before rendering with Opts.EnableSnippets = true below. We can simply set this one render again and check deprecated is set to true.

sammccall accepted this revision.Sep 6 2018, 9:48 AM

Agree with Kadir's comments.
I'd just suggest reducing boilerplate a bit by taking some shortcuts.

clangd/index/Index.h
167

enum class is a pain for bitfields.
I'd just make this a plain enum nested inside symbol, then you don't need to define the operators and don't need to cast as often.

245

FWIW I don't think these accessors pull their weight: in calller code if (Sym.Flags & Symbol::IsDeprecated) is clear enough

clangd/index/Serialization.cpp
309

3

This revision is now accepted and ready to land.Sep 6 2018, 9:48 AM
ioeric updated this revision to Diff 164258.Sep 6 2018, 11:50 AM
ioeric marked 5 inline comments as done.
  • addressed review comments
ioeric updated this revision to Diff 164259.Sep 6 2018, 11:51 AM
  • merged with origin/master
This revision was automatically updated to reflect the committed changes.
kadircet added inline comments.Sep 6 2018, 11:57 AM
unittests/clangd/SymbolCollectorTests.cpp
1021

No need for the optional parameters,

void TestClangc() __attribute__((deprecated));

should work as well