usaxena95 marked an inline comment as done. usaxena95 added inline comments.
================ Comment at: clang-tools-extra/clangd/CollectMacros.h:29 std::vector<Range> Ranges; + llvm::DenseMap<SymbolID, std::set<Location>> MacroRefs; }; ---------------- hokein wrote: > I think the `Ranges` and `MacrosRefs` have a lot of duplications, it is > wasteful to store a same range twice. We don't need the MainFilePath, the > callers should know the corresponding file path for the AST. Something like > `llvm::DenseMap<SymbolID, std::vector<Range>> Refs;` should be enough. > > symbol id for macro generation is required the definition location, but only > defined macros have it, we may need an extra `vector` to store ranges for > undefined macros. > > ``` > #ifndef abc // there is no macro info* for abc, so getSymbolID will fail, but > we still want it, e.g. in semantic highlighting. > #endif > ``` > > There may be more tricky cases, it would be nice to have in the test. > > ``` > #define abc 1 > #undef abc > > #define abc 2 > #undef abc > ``` - using `llvm::DenseMap<SymbolID, std::set<Range>> Refs;` as de-duplication is easier. - Iterating on all the ranges is a pain. Let me know if you want to provide a different method to return all the ranges (sad part: lot of copying). - I had multiple definitions of a macro in the test case, but I have shifted to its separate test case. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70008/new/ https://reviews.llvm.org/D70008 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
