[PATCH] D70489: [clangd] Add xref for macro to static index.
UTKARSH SAXENA via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Dec 2 01:56:03 PST 2019
usaxena95 marked an inline comment as done.
usaxena95 added inline comments.
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:374
+ Roles & static_cast<unsigned>(index::SymbolRole::Definition) ||
+ Roles & static_cast<unsigned>(index::SymbolRole::Reference)))
return true;
----------------
hokein wrote:
> I think we should avoid running the code below if this is a mere reference.
Interesting. Just to understand it better can you give an example where the Role is just a reference ?
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:532
+ const IdentifierInfo *II = MacroRef.first;
+ if (const auto *MI = PP->getMacroDefinition(II).getMacroInfo())
+ if (auto ID = getSymbolID(II->getName(), MI, PP->getSourceManager())) {
----------------
hokein wrote:
> I'm curious of the behavior `getMacroDefinition` -- from its implementation, if `II` doesn't have macro definition, it just returns empty.
>
> could you check whether it works at the below case (or even with a same macro name defined/undef multiple times)?
>
> ```
> #define abc 1
> #undef abc
>
> // not at the EOF, I guess, II for `abc` doesn't have macro definition at this point because it has been undef?
> ```
I had a FIXME in the tests for this case. I see why there was no macro definition at that point. I guess it would be better to keep the symbol id instead of the II.
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.h:76
/// macro even if this is true.
bool CollectMacro = false;
/// Collect symbols local to main-files, such as static functions
----------------
hokein wrote:
> This option is for macro symbols, I'd not use it for collecting macro references. I think the whether to collect macro references is judged by `RefFilter` and `RefsInHeaders`.
I see. `RefFilter` sounds right but I am don't think `RefsInHeaders` is the correct one. It just tells whether to collect references from the included header (i.e. outside mainfile) or not.
I think it would be better to have a separate flag for macro references in such case.
WDYT ?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70489/new/
https://reviews.llvm.org/D70489
More information about the cfe-commits
mailing list