This is an archive of the discontinued LLVM Phabricator instance.

[Index] Add an option to collect macros from preprocesor.
ClosedPublic

Authored by ioeric on Sep 14 2018, 8:06 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ioeric created this revision.Sep 14 2018, 8:06 AM
ioeric updated this revision to Diff 165514.Sep 14 2018, 8:08 AM
  • remove debug message.
ioeric updated this revision to Diff 165516.Sep 14 2018, 8:15 AM
  • another cleanup...

Overall LG, see the major comment about running without the preprocessor.

include/clang/Index/IndexingAction.h
44 ↗(On Diff #165516)

Maybe add a comment or change a name to indicate that this currently only indexes macro definitions?
Macro usages are still only indexed in createIndexingAction.

53 ↗(On Diff #165516)

This does not yet index macro references, maybe keep a mention of this in a comment?

59 ↗(On Diff #165516)

It means we won't have the API to index AST without the preprocessor.
I don't have a strong opinion on whether this change is fine, our usages look ok, but not sure if someone has a use-case that might break.

We could take a slightly more backwards-compatible approach, add an overload without the preprocessor and assert that the IndexMacrosInPreprocessor option is set to false.
Not worth the trouble if all the clients want macros, though. WDYT?

unittests/Index/IndexTests.cpp
30 ↗(On Diff #165516)

Roles and Info are neither tested nor output currently. Consider simplifying the test code by collecting only qual names and using strings everywhere.
More info could always be added when needed.

Feel free to ignore, e.g. if you feel we need this for future revisions.

36 ↗(On Diff #165516)

If we decide to keep Roles and Info, maybe consider outputting them here?
To make sure we don't miss them later in case some tests fail.

61 ↗(On Diff #165516)

Can this actually happen? It seems weird to have a macro occurrence without a MacroInfo.
Maybe try asserting that MI is non-null instead?

ioeric updated this revision to Diff 165761.Sep 17 2018, 7:02 AM
ioeric marked 4 inline comments as done.
  • addressed review comments.
include/clang/Index/IndexingAction.h
44 ↗(On Diff #165516)

Done. Added a comment about this. I think InPreprocessor also indicates this to some extend.

53 ↗(On Diff #165516)

Covered this in the IndexMacrosInPreprocessor option, which seems to be a better place to document this.

59 ↗(On Diff #165516)

yeah, not sure if it's worth the trouble. In theory, a PP should always be available when AST is available (I hope the index library could enforce somehow). And having two overloads with slightly different behaviors seems worse than unknown backward compatibility.

unittests/Index/IndexTests.cpp
30 ↗(On Diff #165516)

Sure.

61 ↗(On Diff #165516)

this can happen for macros that are #undefined. Not relevant anymore.

ilya-biryukov accepted this revision.Sep 18 2018, 12:10 AM

LGTM

include/clang/Index/IndexingAction.h
59 ↗(On Diff #165516)

Agreed, let's see if anyone complains instead of assuming backwards-compatibility is a requirement.

unittests/Index/IndexTests.cpp
61 ↗(On Diff #165516)

Ah, makes sense, thanks.
Any reason to not report those? It looks like a valuable information (e.g. to avoid showing undefined macros in code completion).

This revision is now accepted and ready to land.Sep 18 2018, 12:10 AM
ioeric added inline comments.Sep 18 2018, 1:13 AM
unittests/Index/IndexTests.cpp
61 ↗(On Diff #165516)

I don't think this is a bug e.g. SemaCodeCompletion can intentionally show undefined macros in some contexts. It's up to the index consumer implementations how to handle undefinitions.

This revision was automatically updated to reflect the committed changes.