This is an archive of the discontinued LLVM Phabricator instance.

[include-fixer] Remove line number from Symbol identity
ClosedPublic

Authored by sammccall on Mar 7 2017, 1:24 AM.

Details

Summary

Remove line number from Symbol identity.

For our purposes (include-fixer and clangd autocomplete), function overloads
within the same header should mostly be treated as a single combined symbol.

We may want to track individual occurrences (line number, full type info)
and aggregate this during mapreduce, but that's not done here.

Diff Detail

Repository
rL LLVM

Event Timeline

sammccall created this revision.Mar 7 2017, 1:24 AM
hokein edited edge metadata.Mar 8 2017, 1:18 AM

We may want to track individual occurrences (line number, full type info)
and aggregate this during mapreduce, but that's not done here.

Seems reasonable. So we will introduce a new structure to track these occurrences? Or reuse the SymbolInfo::Signals?

unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
45 ↗(On Diff #90816)

With the change, the name is not a suitable name as it returns to the number of times. Maybe rename it ("symbolSeenTimes"). The same below.

sammccall updated this revision to Diff 90990.Mar 8 2017, 2:33 AM

Address review comments. Uncovered two bugs:

  • accidentally casting through bool: EXPECT_EQ(1, hasSymbol) was a lie
  • we were double counting uses of typedefs where the underlying type was built in, because qualType(hasDeclaration) stops at the typedef in this case.
sammccall marked an inline comment as done.Mar 8 2017, 2:37 AM

We may want to track individual occurrences (line number, full type info)
and aggregate this during mapreduce, but that's not done here.

Seems reasonable. So we will introduce a new structure to track these occurrences? Or reuse the SymbolInfo::Signals?

These aren't really signals, but they're more like map-value than map-key. I suspect it's another new struct. Fortunately we don't actually need it yet.

hokein accepted this revision.Mar 8 2017, 8:28 AM

LGTM once Ben has no other comments. Be careful before submitting it (this patch also changes the interface).

This revision is now accepted and ready to land.Mar 8 2017, 8:28 AM
This revision was automatically updated to reflect the committed changes.