[PATCH] D46943: [clangd] Boost scores for decls from current file in completion

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: clangd/Quality.cpp:24
+  if (SemaCCResult.Declaration)
+    AllDeclsInMainFile = allDeclsInMainFile(SemaCCResult.Declaration);
   if (SemaCCResult.Availability == CXAvailability_Deprecated)
----------------
ioeric wrote:
> ilya-biryukov wrote:
> > sammccall wrote:
> > > ioeric wrote:
> > > > Could you explain why `AllDeclsInMainFile` is necessary? I think it might still be fine to boost score for symbols with a declaration in the main file even if it has redecls in other files (e.g. function fwd in headers).
> > > Agree. I think the better signal is (any) decl in main file.
> > My intuition was that it does not make any sense to functions if there definitions are in the same cpp file, because it does not give any useful signals on whether those should be preferably called in the same file.
> > Also, some defs may not be visible to the compiler at the point of completion and, therefore, won't get a boost, even if they are in the same file. This is inconsistent.
> > 
> > E.g., 
> > 
> > ```
> > // === foo.h
> > class Foo {
> >   int foo();
> >   int bar();
> >   int baz();
> >   int test();
> > };
> > 
> > int glob_foo();
> > int glob_bar();
> > int glob_baz();
> > 
> > // === foo.cpp
> > #include "foo.h"
> > static some_local_func() {}
> > 
> > Foo::foo() {
> > };
> > 
> > int ::glob_foo() {
> > }
> > 
> > Foo::test() {
> >    ^
> >    // out of all functions declared in headers, only foo and global_foo will get a boost here. That does not make much sense, since:
> >    // - glob_bar() and Foo::bar() are also declared in the same file, but compiler hasn't seen them yet, so they won't get a boost.
> >    // - glob_baz() and Foo::baz() are not declared in the same file, but they are so close to `bar` in the header, 
> >    //   and it doesn't seem to make sense to rank them differently.
> > }
> > 
> > Foo::bar() {
> > }
> > 
> > int ::glob_bar() {
> > }
> > ```
> > 
> > Why upranking decls **only** in the current file is still useful?
> >  - Those are usually helpers that are very relevant to the file. Especially true for small files.
> >  - As a side-effect, we'll uprank local vars and parameters (they can't have decls in other files :-)), that seems useful. Maybe such implicit effects are not desirable, though.
> > 
> > I should've definitely documented this better. If we decide this change is useful, I'll add more docs.
> > My intuition was that it does not make any sense to functions if there definitions are in the same cpp file, because it does not give any useful signals on whether those should be preferably called in the same file.
> Not sure if I understand this. Could you explain why?
> 
> > Also, some defs may not be visible to the compiler at the point of completion and, therefore, won't get a boost, even if they are in the same file. This is inconsistent.
> I think this is somewhat expected as that symbol might not be visible to me e.g. a helper function defined after me.
> 
> > out of all functions declared in headers, only foo and global_foo will get a boost here. That does not make much sense, since: ....
> I think you made a very good point here: for a .cc main file, symbols declared/defined in the main header (e.g. `x.h` for `x.cc`) are also interesting and should get a boost, so instead of only boosting symbols that are only declared in the main file, I would suggest also boost symbols in its corresponding header. We would need some heuristics to identify main header though (clang-format uses base file name sans extension which seems to work pretty well).
> 
> Thanks for the example! I think it's very useful for discussions.
> 
> 
> 
>> I think this is somewhat expected as that symbol might not be visible to me e.g. a helper function defined after me.
By "me", I mean I am a decl ;)




Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46943