[PATCH] D143496: [clangd] Add support for missing includes analysis.

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo added a comment.

Thanks for the comments!



================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:712
+          for (auto *Inc : ConvertedIncludes.match(H)) {    
+            if (Pragmas == nullptr || Pragmas->getPublic(Inc->Resolved).empty())
+              Satisfied = true;    
----------------
kadircet wrote:
> kadircet wrote:
> > you can directly use `!Pragmas->isPrivate(Inc->Resolved)` here, instead of getpublic
> this check seems to be new. what's the reason for rejecting private providers? I can see that we might want to be conservative by not inserting private providers, but treating symbols as unsatisfied when a private provider is **already** included doesn't feel right. e.g. the code being analyzed might be allowed to depend on this private header, because it's also part of the library, or it's the public header that's exposing this private header. in such a scenario we shouldn't try to insert the public header again. is there a more concrete issue this code is trying to address?
Ok makes sense. No, I guess I was just confused, because I understood that you wanted a test that includes "private.h" with a diagnostic generated saying that "public.h" should be included instead, so I assumed that was expected behaviour. But that's not what you meant, so I misunderstood.


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:449
+    include_cleaner::Symbol B{*D};
+    syntax::FileRange BRange{SM, B.declaration().getBeginLoc(), 1};
+    include_cleaner::Header Header{*SM.getFileManager().getFile("b.h")};
----------------
kadircet wrote:
> VitaNuo wrote:
> > kadircet wrote:
> > > this is pointing at the declaration inside `b.h` not to the reference inside the main file. are you sure this test passes?
> > Yes, all the tests pass. 
> > `D` is a `Decl` from the main file, otherwise it wouldn't have passed the safeguard ` if (!SM.isWrittenInMainFile(SM.getExpansionLoc(D->getLocation()))) continue;` above.
> this is passing because `bool BDeclFound;` is uninitialized above, if you set it to `bool BDeclFound = false;` you should see the test fail.
> 
> there's no declaration for `b` inside the main file, it's declared in `b.h` and **referenced** inside the main file. you still need to search for the decl (without the constraint of being written in main file), use it to build an include_cleaner::Symbol, and use a `clangd::Annotation` range for the range of the reference.
> 
> it might be easer to write this as:
> ```
> const NamedDecl* B = nullptr;
> for (...) {
>  ...
>  B = D;
> }
> ASSERT_TRUE(B);
> // build expected diagnostic info based on B and check that it's equal to what we've produced
> ```
Didn't know there was a difference between uninitialized and `false`..

Thanks for the idea with `ASSERT_TRUE(Decl)`.  Please check out the new version.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143496/new/

https://reviews.llvm.org/D143496