This is an archive of the discontinued LLVM Phabricator instance.

[Index] Add indexing support for MACROs.
ClosedPublic

Authored by ioeric on Jul 5 2018, 4:23 AM.

Diff Detail

Repository
rC Clang

Event Timeline

ioeric created this revision.Jul 5 2018, 4:23 AM

I worry this is a trap: the indexing infrastructure here is designed so you can run it as a frontendaction, on an ASTUnit, or by passing a set of top level decls.
However the macro functionality necessarily only works when running as a frontend action, so the same consumer would have different semantics when using these functions.
Moreover, clangd does call indexTopLevelDecls(), so this API might actually be awkward for us to use.

Alternatives would be:

  • write lots of loud documentation and hope people read it
  • punt on generalization and just do what we need in clangd with PPCallbacks directly
  • offer a peer API here for consuming macros, and have the indexTopLevelDecls() etc only take the symbol consumer, createIndexingAction() would create both (and you could have a createIndexingPPCallbacks() that takes only the macro consumer).

WDYT?

include/clang/Index/IndexDataConsumer.h
50 ↗(On Diff #154199)

I know this file isn't heavy on documentation, but I think Undefined needs some explanation/example where it might be true. And consider flipping to remove the negation.

Why a default parameter? Virtual + default is slightly confusing, and the #callers here should be few I think.

include/clang/Index/IndexSymbol.h
142

this should take an arg - maybe macroinfo?
There are potentially subkinds here (function-like macros vs object-like ones) and it'll be harder to update callsites once they exist :-)

ioeric updated this revision to Diff 154367.Jul 6 2018, 1:21 AM
ioeric marked 2 inline comments as done.
  • Addressed review comments. Expose create a PPCallbacks for indexing macros.
ioeric added a comment.Jul 6 2018, 1:22 AM

Thanks for the review!

I worry this is a trap: the indexing infrastructure here is designed so you can run it as a frontendaction, on an ASTUnit, or by passing a set of top level decls.
However the macro functionality necessarily only works when running as a frontend action, so the same consumer would have different semantics when using these functions.
Moreover, clangd does call indexTopLevelDecls(), so this API might actually be awkward for us to use.

Alternatives would be:

  • write lots of loud documentation and hope people read it
  • punt on generalization and just do what we need in clangd with PPCallbacks directly
  • offer a peer API here for consuming macros, and have the indexTopLevelDecls() etc only take the symbol consumer, createIndexingAction() would create both (and you could have a createIndexingPPCallbacks() that takes only the macro consumer).

WDYT?

As chatted offline, the fact that indexTopLevelDecls etc can't index macros is unfortunate, but it seems to be by the current design and probably due to how preprocessor and AST work. As you suggested, I added some comments to make this more explicit and also exposed a function that returns a PPCallbacks for indexing macros, which should be useful for clangd's dynamic index. Hope this would make these functions harder to misuse.

sammccall added inline comments.Jul 6 2018, 2:08 AM
include/clang/Index/IndexDataConsumer.h
48 ↗(On Diff #154367)

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.

include/clang/Index/IndexingAction.h
44

nit: indexes all symbols (macros and AST decls).
i.e. if there were other types in the future, they'd get indexed too.

51

nit: can you break before the "Note" here and below, to make this easier to find?

62

I found this second part hard to understand - I think it's what I'd expect (Preprocessor knows about PPCallbacks, but not the other way around), but I had to look up what "set the preprocessor" meant.

Maybe "The caller is responsible for calling Consumer.setPreprocessor()"?

67

are you sure it doesn't index macros? is this a bug? maybe this should be FIXME instead of a note

lib/Index/IndexSymbol.cpp
357

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.

lib/Index/IndexingAction.cpp
49

making these shared_ptr everywhere feels a bit like giving up :-)
Can't we keep this one and IndexPPCallbacks as references?

ioeric updated this revision to Diff 154388.Jul 6 2018, 4:53 AM
ioeric marked 6 inline comments as done.
  • addressed review comments.
ioeric added inline comments.Jul 6 2018, 4:53 AM
include/clang/Index/IndexDataConsumer.h
48 ↗(On Diff #154367)

Sounds good. Done.

include/clang/Index/IndexingAction.h
67

It looks like so from the current implementation:

for (const Decl *D : Reader.getModuleFileLevelDecls(Mod)) {
  IndexCtx.indexTopLevelDecl(D);
}

Changed to FIXME.

lib/Index/IndexSymbol.cpp
357

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.

lib/Index/IndexingAction.cpp
49

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.

ioeric updated this revision to Diff 154390.Jul 6 2018, 5:07 AM
  • removed Undefined parameter.
sammccall accepted this revision.Jul 9 2018, 12:07 AM
sammccall added inline comments.
include/clang/Index/IndexDataConsumer.h
48 ↗(On Diff #154390)

the change from pointer->reference seems maybe unneccesary, and inconsistent with other callbacks? up to you though, doesn't seem like a big deal either way.

include/clang/Index/IndexSymbol.h
105

maybe mention that macro occurrences aren't currently reported by libclang? as it's related.

lib/Index/IndexingContext.cpp
422

this should be Undefinition (and drop the Undefined flag).

This revision is now accepted and ready to land.Jul 9 2018, 12:07 AM
ioeric updated this revision to Diff 154543.Jul 9 2018, 1:13 AM
ioeric marked 2 inline comments as done.
  • addressed review comments.
This revision was automatically updated to reflect the committed changes.

This changset is not handling #ifdef MACRO, defined(MACRO) etc. in he PPCallbacks. Is there any intention to omit them or do you simply forgot them?

Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2019, 6:11 AM
Herald added a subscriber: dexonsmith. · View Herald Transcript