usaxena95 added inline comments.
================
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:
> usaxena95 wrote:
> > 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.
> sorry, I didn't see a FIXME in the test, am I missing anything? Maybe move
> the FIXME to here?
Instead of storing the II, I now store the SymbolID. So we do not need to call
`getMacroDefinition` at the end of TU.
Therefore the problem with undef and multiple declaration of macros was
resolved and I removed the FIXME too.
There is a test of the form
```
// Macro defined multiple times.
#define $ud1[[UD]] 1
int ud_1 = $ud1[[UD]];
#undef UD
#define $ud2[[UD]] 2
int ud_2 = $ud2[[UD]];
#undef UD
```
================
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:
> usaxena95 wrote:
> > 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 ?
> I don't think we need an extra flag merely for macro references. macro
> references should be treated in the same way of decl references.
Got it. Done.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70489/new/
https://reviews.llvm.org/D70489
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
