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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
