kadircet added a comment.
thanks, looks great!
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:451
+ tooling::HeaderIncludes HeaderIncludes(AST.tuPath(), Code,
+ tooling::IncludeStyle{});
+ const SourceManager &SM = AST.getSourceManager();
----------------
we should respect the style configurations (sorry for missing this in previous
iterations).
you can get the relevant style with: `clang::format::getStyle`, which has an
[IncludeStyle](https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Format/Format.h#L2188).
in case the `getStyle` fails, we should fallback to
`clang::format::getLLVMStyle` as we do in other places. you can get at the
relevant VFS instance through sourcemanager.
================
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;
----------------
you can directly use `!Pragmas->isPrivate(Inc->Resolved)` here, instead of
getpublic
================
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:
> 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?
================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:443
+ std::string Name =
llvm::dyn_cast<NamedDecl>(D)->getQualifiedNameAsString();
+ if (Name != "b") {
+ continue;
----------------
nit: braces
================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:456
+
+// TODO(bakalova) examples with std::vector and IWYU private pragma still
needed
+TEST(IncludeCleaner, GenerateMissingHeaderDiags) {
----------------
i think the example for `std::vector` is solid, and `IWYU pragma private` needs
a little adjustment.
================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:469
+#include "header.h"
+#include "private.h"
+$insert_f[[]]$insert_vector[[]]
----------------
we should include private.h through some indirection (not public.h) to check
`IWYU pragma private` spellings are respected.
================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:475
+
+ ns::$d[[Bar]] bar;
+ bar.d();
----------------
name this range as `bar` instead of `d`?
================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:479
+
+ buzz();
+
----------------
could you add a comment here saying this shouldn't be diagnosed?
================
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")};
----------------
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
```
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
