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

Reply via email to