hokein added inline comments.
================ Comment at: clang-tools-extra/clangd/ParsedAST.cpp:609 + Config::UnusedIncludesPolicy::Experiment) + Pragmas.record(*Clang); // Copy over the macros in the preamble region of the main file, and combine ---------------- kadircet wrote: > hokein wrote: > > kadircet wrote: > > > why do we need to collect pragmas in main file? i think we already have > > > necessary information available via `IncludeStructure` (it stores keeps > > > and exports, and we don't care about anything else in the main file > > > AFAICT). so let's just use the PragmaIncludes we're getting from the > > > Preamble directly? without even making a copy and returning a reference > > > from the `Preamble` instead in `ParsedAST::getPragmaIncludes` > > > i think we already have necessary information available via > > > IncludeStructure (it stores keeps and exports, and we don't care about > > > anything else in the main file AFAICT) > > > > The IncludeStructure doesn't have a full support for IWYU export pragma, it > > only tracks the headers that have the export pragma. > > > > My understand of the end goal is to use the `PragmaInclude` to handle every > > IWYU-related things, and we can remove all these IWYU bits in the > > `IncludeStructure`, clangd IWYU pragma handling > > [code](https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/Headers.cpp#L127-L166). > > The IncludeStructure doesn't have a full support for IWYU export pragma, it > > only tracks the headers that have the export pragma. > > And I think that's all we need for IWYU pragmas inside the main file (as main > file is a leaf and exporting headers from it doesn't change anything apart > from making sure we keep them around) > > > My understand of the end goal is to use the PragmaInclude to handle every > > IWYU-related things, and we can remove all these IWYU bits in the > > IncludeStructure, clangd IWYU pragma handling code. > > Yes, I agree with some version of that, but until then this is introducing > some extra changes (+ copying around more information in every AST build). So > can we leave this piece out of the initial patch and just use the > PragmaInclude we have from Preamble without copying it around? > Yes, I agree with some version of that, but until then this is introducing > some extra changes (+ copying around more information in every AST build). So > can we leave this piece out of the initial patch and just use the > PragmaInclude we have from Preamble without copying it around? Sure, that sounds a good plan, and have a better narrowed scope of the patch. ================ Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:345 auto Unused = computeUnusedIncludes(AST); EXPECT_THAT(Unused, ElementsAre(Pointee(writtenInclusion("<queue>")))); + EXPECT_THAT(computeUnusedIncludesNew(AST), ---------------- kadircet wrote: > can you inline the call to `computeUnusedIncludes` into the EXPECT statement > here? sure, since this is not directly related to this patch, addressed in ccb67491f0dd55c5bd8ed5f71cb802422bfaa969. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140875/new/ https://reviews.llvm.org/D140875 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
