[PATCH] D48961: [Index] Add indexing support for MACROs.

[Eric Liu via Phabricator via cfe-commits]

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