[SemaCodeComplete] Allow passing out scope specifiers in qualified-id completions via completion context.
ClosedPublic

Authored by ioeric on Tue, Nov 28, 8:45 AM.

Diff Detail

Repository
rL LLVM
ioeric created this revision.Tue, Nov 28, 8:45 AM
nik added a subscriber: nik.Wed, Nov 29, 12:28 AM
ilya-biryukov added inline comments.Wed, Nov 29, 5:18 AM
include/clang/Sema/CodeCompleteConsumer.h
284 ↗(On Diff #124584)

Maybe add a brief comment for consistency with other decls?

lib/Sema/SemaCodeComplete.cpp
4609 ↗(On Diff #124584)

Why do we alter this code path?

4609 ↗(On Diff #124584)

Maybe we should add a test or provide examples why the current code does not work for us?

4611 ↗(On Diff #124584)

Do we really want to set invalid scope specifiers here?

ioeric updated this revision to Diff 124739.Wed, Nov 29, 6:31 AM
ioeric marked 4 inline comments as done.
  • Address review comments.
lib/Sema/SemaCodeComplete.cpp
4609 ↗(On Diff #124584)

There is no existing unit test for CodeCompletion, and there is no much to test here either. I added a comment with example for this.

4611 ↗(On Diff #124584)

Yes, see the comment I added.

Could you please add a test?

Could you please add a test?

Any tip on how this should be tested? I couldn't find any existing unit test for either SemaCodeComplete or code completion context (under clang/unittests). It might be possible to set up a unit test framework for code completion, but I'm not sure if it's worth to do so since this change doesn't change completion behavior.

If nothing uses getCXXScopeSpecifier right now we can't really test it with a clang or c-index-test regression test. A completion unit test could work here. I don't think we actually have existing completion unit tests though, so you would have to create one from scratch. But if getCXXScopeSpecifier will be used in a follow up patch maybe it will be easier to commit this without a test together with the followup patch?

ilya-biryukov added inline comments.Thu, Nov 30, 1:40 AM
lib/Sema/SemaCodeComplete.cpp
4609 ↗(On Diff #124584)

Makes sense, thanks for the explanation.

If nothing uses getCXXScopeSpecifier right now we can't really test it with a clang or c-index-test regression test. A completion unit test could work here. I don't think we actually have existing completion unit tests though, so you would have to create one from scratch. But if getCXXScopeSpecifier will be used in a follow up patch maybe it will be easier to commit this without a test together with the followup patch?

I have another clangd patch that uses this, but this would still need to be a separate patch since they are in different repos...

arphaman accepted this revision.Thu, Nov 30, 4:34 PM

If nothing uses getCXXScopeSpecifier right now we can't really test it with a clang or c-index-test regression test. A completion unit test could work here. I don't think we actually have existing completion unit tests though, so you would have to create one from scratch. But if getCXXScopeSpecifier will be used in a follow up patch maybe it will be easier to commit this without a test together with the followup patch?

I have another clangd patch that uses this, but this would still need to be a separate patch since they are in different repos...

You can commit the two in one either using the monrepo or sparse SVN checkout ;)
But I wouldn't object if you commit this separately right before the clangd patch, so LGTM

This revision is now accepted and ready to land.Thu, Nov 30, 4:34 PM
ioeric added a comment.Thu, Dec 7, 8:46 AM

Fyi, you could find use of the new API in D40548

I'd like to land this patch if there is no objection.

This revision was automatically updated to reflect the committed changes.