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

[Sam McCall via Phabricator via cfe-commits]

sammccall 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.
----------------
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.


================
Comment at: include/clang/Index/IndexingAction.h:44
 
+/// Creates a frontend action that indexes macros and AST decls.
 /// \param WrappedAction another frontend action to wrap over or null.
----------------
nit: indexes all symbols (macros and AST decls).
i.e. if there were other types in the future, they'd get indexed too.


================
Comment at: include/clang/Index/IndexingAction.h:51
 
+/// Recursively indexes all decls in the AST. Note that this does not index
+/// macros.
----------------
nit: can you break before the "Note" here and below, to make this easier to find?


================
Comment at: include/clang/Index/IndexingAction.h:61
+/// Creates a PPCallbacks that indexes macros and feeds macros to \p Consumer.
+/// Note that this does not set the preprocesssor in \p Consumer. If \p Consumer
+/// needs Preprocessor, it has to be set separately before callback is invoked.
----------------
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()"?


================
Comment at: include/clang/Index/IndexingAction.h:66
+
+/// Recursively indexes all top-level decls in the module. Note that this does
+/// not index macros.
----------------
are you sure it doesn't index macros? is this a bug? maybe this should be FIXME instead of a note


================
Comment at: lib/Index/IndexSymbol.cpp:357
+  Info.Properties = SymbolPropertySet();
+  // FIXME: set languages more accurately.
+  Info.Lang = SymbolLanguage::C;
----------------
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.


================
Comment at: lib/Index/IndexingAction.cpp:52
   std::shared_ptr<Preprocessor> PP;
-  IndexingContext &IndexCtx;
+  std::shared_ptr<IndexingContext> IndexCtx;
 
----------------
making these shared_ptr everywhere feels a bit like giving up :-)
Can't we keep this one and IndexPPCallbacks as references?


Repository:
  rC Clang

https://reviews.llvm.org/D48961