kadircet added inline comments.
================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:479 + } + // FIXME: !!this is a hacky way to collect macro references. + std::vector<include_cleaner::SymbolReference> Macros; ---------------- hokein wrote: > kadircet wrote: > > this might behave slightly different than what we have in `RecordedPP`, and > > rest of the applications of include-cleaner will be using that. can we > > expose the pieces in RecordedPP that collects MacroRefs as a separate > > handler and attach that collector (combined with the skipped range > > collection inside `CollectMainFileMacros` and also still converting to > > `MainFileMacros` in the end (as we can't store > > sourcelocations/identifierinfos from preamble)? > > > > afterwards we can use the names stored in there to get back to > > `IdentifierInfo`s and Ranges stored to get back to `SourceLocation`s. WDYT? > Yeah, this is a better solution, but I'm not sure whether we should do this > before the release cut, it has a side effect of changing the > find-macro-reference behavior in clangd. It requires some design/implement > work. > > I agree that the current solution is hacky, and eventually will be replaced, > but it follows the existing `findReferencedMacros`, so it is not that bad. I > tend to land this version before the release cut. What do you think? > Yeah, this is a better solution, but I'm not sure whether we should do this > before the release cut, it has a side effect of changing the > find-macro-reference behavior in clangd. OK, i think you're right about possibly changing semantic highlighting&refs for clangd. Let's push this as a follow-up after the branch cut then. > It requires some design/implement work. No action needed here just wanted to layout some ideas; We just need to change the implementation details of CollectMainFileMacros to wrap the RecordedPP and add skipped ranges support to it. We might need to do a little plumbing to convert between types (as RecordedPP stores SourceLocations/MacroInfos that can't be used across preamble and AST) but that should be trivial to marshal (we can do the conversion inside EndOfMainFile). I haven't fully tried this though, so if you tried and faced certain incompatibilities PLMK. > I agree that the current solution is hacky, and eventually will be replaced, > but it follows the existing findReferencedMacros, so it is not that bad. I > tend to land this version before the release cut. What do you think? I am worried that landing this version and letting people use it for 6 months might result in us getting bug reports that we can't address until we converge (or even worse get bug reports due to behavior change after we converge). But changes here are somewhat more justified as we put this as an experimental feature, compared to regressions in existing clangd behavior as you pointed out. ================ Comment at: clang-tools-extra/clangd/IncludeCleaner.h:100 std::vector<const Inclusion *> computeUnusedIncludes(ParsedAST &AST); +std::vector<const Inclusion *> computeUnusedIncludesNew(ParsedAST &AST); ---------------- s/computeUnusedIncludesNew/computeUnusedIncludesExperimental/ can you also add a comment saying that this uses include-cleaner library to perform usage analysis? ================ 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 ---------------- 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? ================ 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), ---------------- can you inline the call to `computeUnusedIncludes` into the EXPECT statement here? ================ Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:382 UnusedIncludes.push_back(Include->Written); EXPECT_THAT(UnusedIncludes, UnorderedElementsAre("\"unused.h\"", "\"dir/unused.h\"")); ---------------- can you also update the test here to have same structure as below? 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
