New name suggestions were being used in places where existing names should have been used, this patch tries to fix some of those situations.
Details
Diff Detail
- Repository
- rC Clang
- Build Status
Buildable 24128 Build 24127: arc lint + arc unit
Event Timeline
lib/Sema/SemaCodeComplete.cpp | ||
---|---|---|
4839 | I'm not sure statement is correct here. Secondly, ISTM there are other cases we might have a qualified ID in a non-expression context, like implementing a function void foo::bar() or so? Are we sure none of those hit this codepath? No need to fix this now (this code is clearly an improvement) but maybe this is worth a comment like // Usually qualified-IDs occur in expression context or so. | |
4897–4900 | Hmm, I don't think CCC_Namespace is right either. This is a using declaration, not a using directive. I'd suggest either adding a new CCC enum value for this (PotentiallyQualifiedName is actually the best name for this! we'd need to come up with a new one), or use Expression with a comment that this isn't really accurate and someone should fix it :-) |
There are still some contexts where both a new name and an existing name is permissible, going to add a comment on those to leave some traces for the next person who cares
include/clang/Sema/CodeCompleteConsumer.h | ||
---|---|---|
277 | It's not obvious what "name" means here, e.g. what distinguishes this from "expression". I think the answer is "we want the name of *something*, but we don't know what". I think it's ok to say that explicitly. Or to call this CCC_Symbol, etc. | |
281 | what distinguishes potentiallyqualifiedexistingname from existingname? The one use of ExistingName is the invalid scope case. This seems a) pretty marginal, and b) not to matter to clangd. Does it matter to libclang? (The behavior is detectable through the API, but the current behavior doesn't seem that sensible or obviously important) |
include/clang/Sema/CodeCompleteConsumer.h | ||
---|---|---|
277 | Renaming to CCC_Symbol | |
281 | It kind of matters to libclang, but it is not that important as well. It is used when caching completion items, to decide whether they allow a name specifiers or not. But when I think about the use case, I believe nested name specifiers should be allowed in that context(when scope is invalid, but we still report for global code completion). So seems like it is redundant(not exactly, but currently we have no way of telling it apart due to mentioned fixme). |
Logic looks good. Any way we can exercise it with a test via c-index-test?
include/clang/Sema/CodeCompleteConsumer.h | ||
---|---|---|
281 | nit: try to describe what a symbol is in the comment, rather than just repeating the term? |
Since it doesn't change any CXCompletionContext's in CIndex it doesn't seem possible. There are tests being added to clangd(D53192), since it uses context provided by sema to decide whether query the index or not, we can check the behavior.
I've bisected https://bugs.llvm.org/show_bug.cgi?id=42646 to this change. Reverting it fixes the issue.
Please look into it.
It's not obvious what "name" means here, e.g. what distinguishes this from "expression".
I think the answer is "we want the name of *something*, but we don't know what". I think it's ok to say that explicitly. Or to call this CCC_Symbol, etc.