[PATCH] D147034: [clangd] Replace the hacky include-cleaner macro-reference implementation.

[Haojian Wu via Phabricator via cfe-commits]

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