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