ioeric added inline comments.
================
Comment at: include/clang/Index/IndexDataConsumer.h:48
/// \returns true to continue indexing, or false to abort.
- virtual bool handleMacroOccurence(const IdentifierInfo *Name,
- const MacroInfo *MI, SymbolRoleSet Roles,
- SourceLocation Loc);
+ /// \p Undefined is set when the occurrence is in an #undef directive where
+ /// the macro undefined.
----------------
sammccall wrote:
> Ah, now I understand :-)
>
> This doesn't seem like another orthogonal bool parameter, it seems more like
> a SymbolRole (we have Definition there, logically Undefinition seems like it
> should be there too, even if it only applies to some symbols).
>
> It looks like it would be safe to insert this as bit 9, and move all the
> others down by 1 bit? Only the libclang API needs to be stable, and that ends
> at bit 8.
Sounds good. Done.
================
Comment at: include/clang/Index/IndexingAction.h:66
+
+/// Recursively indexes all top-level decls in the module. Note that this does
+/// not index macros.
----------------
sammccall wrote:
> are you sure it doesn't index macros? is this a bug? maybe this should be
> FIXME instead of a note
It looks like so from the current implementation:
```
for (const Decl *D : Reader.getModuleFileLevelDecls(Mod)) {
IndexCtx.indexTopLevelDecl(D);
}
```
Changed to `FIXME`.
================
Comment at: lib/Index/IndexSymbol.cpp:357
+ Info.Properties = SymbolPropertySet();
+ // FIXME: set languages more accurately.
+ Info.Lang = SymbolLanguage::C;
----------------
sammccall wrote:
> I'm not sure what the intent of this comment is - what's the behavior you
> *want* here?
> One principled option is to introduce a new SymbolLanguage for the
> preprocessor (it's reasonable to consider this a separate language).
> Another is to always consider macros C symbol, but then this shouldn't be a
> fixme.
> I'm not sure what the intent of this comment is - what's the behavior you
> *want* here?
I'm not really sure what I want... On one hand, it's possible to get the actual
language from `LangOptions` of the compiler instance; on the other hand, the
index library doesn't seem to care about `LangOptions` for decls (maybe
intentionally?), and I'm a bit hesitant to fix them.
> Another is to always consider macros C symbol, but then this shouldn't be a
> fixme.
Sounds good. This aligns with the current behavior of the library. Removed
FIXME.
================
Comment at: lib/Index/IndexingAction.cpp:52
std::shared_ptr<Preprocessor> PP;
- IndexingContext &IndexCtx;
+ std::shared_ptr<IndexingContext> IndexCtx;
----------------
sammccall wrote:
> making these shared_ptr everywhere feels a bit like giving up :-)
> Can't we keep this one and IndexPPCallbacks as references?
It's unclear how exactly `IndexingContext` should be used :( In the current
design, the context is owned by `IndexActionBase` and referenced/shared by AST
consumer and PPCallbacks for the IndexAction use case. The new
`indexMacroCallbacks` function would also need to create a context that needs
to be owned by PPCallbacks. Ideally, the ASTConsumer and PPCallbacks can own
their own context, but it's unclear whether this would fit into the design of
IndexingContext. So `shared_ptr` seems to be reasonable given the current
design.
Repository:
rC Clang
https://reviews.llvm.org/D48961
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
