This is an archive of the discontinued LLVM Phabricator instance.

[CodeComplete] Expose InBaseClass signal in code completion results.
ClosedPublic

Authored by ioeric on Oct 24 2018, 2:59 AM.

Details

Summary

No new tests as the existing tests for result priority should give us
coverage. Also as the new flag is trivial enough, I'm reluctant to plumb the
flag to c-index-test output.

Diff Detail

Repository
rL LLVM

Event Timeline

ioeric created this revision.Oct 24 2018, 2:59 AM
ilya-biryukov added a comment.EditedOct 24 2018, 3:10 AM

LG, but could we add a test for the new flag it by printing it in PrintingCodeCompleteConsumer::ProcessCodeCompleteResults() and adding corresponding tests to clang/test/CodeCompletion?

Similar to how it's done for the Hidden flag in the results

ioeric updated this revision to Diff 170838.Oct 24 2018, 3:35 AM
  • Add tests for the new flag.

LG, but could we add a test for the new flag it by printing it in PrintingCodeCompleteConsumer::ProcessCodeCompleteResults() and adding corresponding tests to clang/test/CodeCompletion?

Similar to how it's done for the Hidden flag in the results

Done. That was way easy (comparing to c-index-test). Thanks for the pointers!

ilya-biryukov accepted this revision.Oct 24 2018, 5:05 AM

LGTM with a NIT

lib/Sema/CodeCompleteConsumer.cpp
557 ↗(On Diff #170838)

NIT: Maybe add a space between the items, i.e. join with ", " as a separator?

This revision is now accepted and ready to land.Oct 24 2018, 5:05 AM

And another NIT :-)

lib/Sema/CodeCompleteConsumer.cpp
548 ↗(On Diff #170838)

NIT: maybe move Tags into the corresponding case handler?

Would require putting a set of braces around it, but that shouldn't make the code less readable.

ioeric updated this revision to Diff 170854.Oct 24 2018, 5:44 AM
ioeric marked an inline comment as done.
  • move tags into case.
ioeric updated this revision to Diff 170855.Oct 24 2018, 5:45 AM
  • Rebase
lib/Sema/CodeCompleteConsumer.cpp
557 ↗(On Diff #170838)

Considering we put everything in one line, I think it's reasonable to drop the spaces to keep the output compact. These tags are more expected to be read by tests than human after all.

This revision was automatically updated to reflect the committed changes.
ilya-biryukov added inline comments.Oct 24 2018, 9:53 AM
lib/Sema/CodeCompleteConsumer.cpp
557 ↗(On Diff #170838)

I would argue this output is also read by humans, therefore we should add a space.
We do "grep" the results with FileCheck in tests and technically it does not matter if we have a space or not. But the users still look at the output when (1) something goes wrong or (2) they read through the test cases.