Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

textDocument/implementation doesn't resolve local classes #644

Closed
njames93 opened this issue Jan 13, 2021 · 6 comments
Closed

textDocument/implementation doesn't resolve local classes #644

njames93 opened this issue Jan 13, 2021 · 6 comments
Assignees

Comments

@njames93
Copy link

njames93 commented Jan 13, 2021

Take this annotated example, where implementation is called on the points $^ and returns the corresponding ranges $[[]]

class $Class^A {
public:
  virtual ~A();
  virtual void $Method^doSomething();
};

class $Class[[B]] : A {
public:
  void $Method[[doSomething]]() override {}
};

namespace Other {
class $Class[[C]] : A {
public:
  void $Method[[doSomething]]() override {}
};
} // namespace Other

void foo() {
  class D : A {
  public:
    void doSomething() override {}
  };
}

As can be seen, the implementation(D) in foo is not returned.
This behaviour appears to happen when everything is defined in the same file, or if the implementations are in their own cpp file.

@HighCommander4
Copy link

Type hierarchy has the same issue.

@usx95
Copy link

usx95 commented Jan 14, 2021

Thanks for filing this.
Both textDocument/typeHierarchy(for subclasses) and textDocument/implementation are driven by the index.
Currently the index does not collect local symbols (such as those declared inside a FunctionDecl, BlockDecl,..).
This was done primarily to reduce the size of the index and thus the results are not entirely accurate in such a case.

@njames93
Copy link
Author

Would it be wise to index just polymorphic classes inside functions, blocks, lambdas etc. There may be a case to index all records, but i feel there could be too much of a penalty there.

@hokein
Copy link
Contributor

hokein commented Jan 15, 2021

As discussed offline with @usx95, indexing function-local classes (and its methods) looks like a reasonable fix.
Defining a class inside function body is not a super common pattern, so the increase of memory should not be large.

I think it is worth to try this solution and see how it goes (measuring the size of the index and the performance of the index).

@usx95
Copy link

usx95 commented Jan 15, 2021

Sent https://reviews.llvm.org/D94785 for review.

@njames93
Copy link
Author

Thanks for fixing this 👍

arichardson pushed a commit to arichardson/llvm-project that referenced this issue Mar 30, 2021
Previously we did not record local class declarations. Now with features like
findImplementation and typeHierarchy, we have a need to index such local
classes to accurately report subclasses and implementations of methods.

Performance testing results:
- No changes in indexing timing.
- No significant change in memory usage.
- **1%** increase in #relations.
- **0.17%** increase in #refs.
- **0.22%** increase #symbols.

**New index stats**
Time to index: **4:13 min**
memory usage **543MB**
number of symbols: **521.5K**
number of refs: **8679K**
number of relations: **49K**

**Base Index stats**
Time to index: **4:15 min**
memory usage **542MB**
number of symbols: **520K**
number of refs: **8664K**
number of relations: **48.5K**

Fixes: clangd/clangd#644

Differential Revision: https://reviews.llvm.org/D94785
mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this issue Oct 7, 2022
Previously we did not record local class declarations. Now with features like
findImplementation and typeHierarchy, we have a need to index such local
classes to accurately report subclasses and implementations of methods.

Performance testing results:
- No changes in indexing timing.
- No significant change in memory usage.
- **1%** increase in #relations.
- **0.17%** increase in #refs.
- **0.22%** increase #symbols.

**New index stats**
Time to index: **4:13 min**
memory usage **543MB**
number of symbols: **521.5K**
number of refs: **8679K**
number of relations: **49K**

**Base Index stats**
Time to index: **4:15 min**
memory usage **542MB**
number of symbols: **520K**
number of refs: **8664K**
number of relations: **48.5K**

Fixes: clangd/clangd#644

Differential Revision: https://reviews.llvm.org/D94785
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants