ioeric added inline comments.
================
Comment at: clangd/Quality.cpp:24
+ if (SemaCCResult.Declaration)
+ AllDeclsInMainFile = allDeclsInMainFile(SemaCCResult.Declaration);
if (SemaCCResult.Availability == CXAvailability_Deprecated)
----------------
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.
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
