kadircet added a comment.
thanks!
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:143
+ std::vector<include_cleaner::SymbolReference> Macros;
+ for (const auto &MAndRefs : AST.getMacros().MacroRefs) {
+ for (const auto &Ref : MAndRefs.second) {
----------------
nit: `const auto &[SID, Refs]`
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:145
+ for (const auto &Ref : MAndRefs.second) {
+ auto L = sourceLocationInMainFile(SM, Ref.Rng.start);
+ if (!L) {
----------------
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:150
+ }
+ if (const auto *Tok =
+ AST.getTokens().spelledTokenAt(*L)) {
----------------
nit: early exit instead
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:156
+
+ if (auto DefLoc = Macro->Info->getDefinitionLoc(); DefLoc.isValid())
+ Macros.push_back(
----------------
we should use `Macro->NameLoc`, not the definition loc inside macro-info
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:162
+ Ref.InConditionalDirective
+ ? include_cleaner::RefType::Implicit
+ : include_cleaner::RefType::Explicit});
----------------
i believe you meant `Ambigious` here. the reference is "explicitly" in the
code, but we can't say for sure if it's intended.
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
