hokein added a comment. thanks for the comments.
================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:145 + for (const auto &Ref : MAndRefs.second) { + auto L = sourceLocationInMainFile(SM, Ref.Rng.start); + if (!L) { ---------------- kadircet wrote: > it's unfortunate that we're scanning the file for every occurrence of a macro > just to get an offset. this might get problematic in big test files, what > about introducing an offset to `MacroOccurence`? > it's unfortunate that we're scanning the file for every occurrence of a macro > just to get an offset. this might get problematic in big test files, what > about introducing an offset to `MacroOccurence`? ================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:145 + for (const auto &Ref : MAndRefs.second) { + auto L = sourceLocationInMainFile(SM, Ref.Rng.start); + if (!L) { ---------------- hokein wrote: > kadircet wrote: > > it's unfortunate that we're scanning the file for every occurrence of a > > macro just to get an offset. this might get problematic in big test files, > > what about introducing an offset to `MacroOccurence`? > > it's unfortunate that we're scanning the file for every occurrence of a > > macro just to get an offset. this might get problematic in big test files, > > what about introducing an offset to `MacroOccurence`? > > thanks, it makes sense, sent out https://reviews.llvm.org/D153259. ================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:156 + + if (auto DefLoc = Macro->Info->getDefinitionLoc(); DefLoc.isValid()) + Macros.push_back( ---------------- kadircet wrote: > we should use `Macro->NameLoc`, not the definition loc inside macro-info oh, right. NameLoc takes the preamble patch into account. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147034/new/ https://reviews.llvm.org/D147034 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits