This is an archive of the discontinued LLVM Phabricator instance.

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

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

Event Timeline

ioeric created this revision.Nov 28 2017, 8:45 AM
nik added a subscriber: nik.Nov 29 2017, 12:28 AM
ilya-biryukov added inline comments.Nov 29 2017, 5:18 AM
include/clang/Sema/CodeCompleteConsumer.h
284

Maybe add a brief comment for consistency with other decls?

lib/Sema/SemaCodeComplete.cpp
4609

Why do we alter this code path?

4609

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

4611

Do we really want to set invalid scope specifiers here?

ioeric updated this revision to Diff 124739.Nov 29 2017, 6:31 AM
ioeric marked 4 inline comments as done.
  • Address review comments.
lib/Sema/SemaCodeComplete.cpp
4609

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

Yes, see the comment I added.

arphaman edited edge metadata.Nov 29 2017, 12:41 PM

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.Nov 30 2017, 1:40 AM
lib/Sema/SemaCodeComplete.cpp
4609

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.Nov 30 2017, 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.Nov 30 2017, 4:34 PM
ioeric added a comment.Dec 7 2017, 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.