Page MenuHomePhabricator

[include-fixer] Add a prototype for a new include fixing tool.
ClosedPublic

Authored by bkramer on Apr 20 2016, 3:07 AM.

Details

Summary

The goal of this tool is fairly simple, look up unknown identifiers in a
global database and add the corresponding #include line. It accomplishes
this by hooking into Sema as an ExternalSemaSource and responding to typo
correction callbacks. This means we can see the unknown identifier before
it's being munged by error recovery.

This doesn't work perfectly yet as some typo corrections don't emit
callbacks (delayed typos), but I think this is fixable. We also handle
only one include at a time as this is meant to be run directly from
the editing environment eventually. Adding multiple includes at the same
time is tricky because of error recovery.

This version only has a a dummy database, so all you can do is fixing
missing includes of <string>, but the indexer to build a database will
follow soon.

Diff Detail

Repository
rL LLVM

Event Timeline

bkramer updated this revision to Diff 54334.Apr 20 2016, 3:07 AM
bkramer retitled this revision from to [include-fixer] Add a prototype for a new include fixing tool..
bkramer updated this object.
bkramer added a reviewer: djasper.
bkramer added subscribers: cfe-commits, hokein, ioeric.
klimek added a subscriber: klimek.Apr 20 2016, 3:21 AM
klimek added inline comments.
include-fixer/FixedXrefsDB.h
18 ↗(On Diff #54334)

Add '.', remove "intended for testing". I don't think telling people to not use it outside of testing helps a lot. For example, in the compilation database we found that it helps to allow people to pass a fixed-compilation-db at the command line; similar things might make sense here.

20 ↗(On Diff #54334)

I'd make it constructible from a map<string, vector<string>>. Especially for tests, I'd want to see the test setup.

include-fixer/IncludeFixer.cpp
117 ↗(On Diff #54334)

specification, *,*

132 ↗(On Diff #54334)

Can we sort this so the public interface comes first? Also, why is the public interface so large?

179 ↗(On Diff #54334)

This is not actually rewriting, but constructing the replacements, right?

184–190 ↗(On Diff #54334)

Can we reuse functionality from clang-format here?

include-fixer/IncludeFixer.h
26 ↗(On Diff #54334)

Why are we passing ownership?

36 ↗(On Diff #54334)

That also seems weird in the interface here.

bkramer updated this revision to Diff 54342.Apr 20 2016, 4:38 AM
  • FixedXrefsDB -> FakeXrefsDB
  • FakeXrefsDB is now configurable
  • Cosmetic & comment fixes.
  • Moved query to private methods.
  • Always insert new includes at the top, let clang-format sort out the rest (not yet implemented in the tool, pending fixReplacements)
  • Don't take ownership of XrefsDB
bkramer updated this revision to Diff 54344.Apr 20 2016, 4:47 AM

From now on you shall be known as InMemoryXrefsDB.

bkramer marked 8 inline comments as done.Apr 20 2016, 5:07 AM
bkramer added inline comments.
include-fixer/IncludeFixer.cpp
132 ↗(On Diff #54334)

Sorted. Most of it is dealing with clang callbacks. I also don't consider this to be a public interface as it's all in the cpp file.

184–190 ↗(On Diff #54334)

Yes we should. I changed it to insert new includes before the first include, clang-format will clean up the rest. clang-format isn't wired up to the tool yet, I'd like to use the new applyAllReplacements with formatting tools for that, should be ready soon.

include-fixer/IncludeFixer.h
26 ↗(On Diff #54334)

Taking a reference now.

36 ↗(On Diff #54334)

Removed.

klimek accepted this revision.Apr 20 2016, 5:18 AM
klimek added a reviewer: klimek.

Cool, lg.

include-fixer/IncludeFixer.cpp
133 ↗(On Diff #54344)

Ah, right, didn't see that it's all in the cpp.

This revision is now accepted and ready to land.Apr 20 2016, 5:18 AM
This revision was automatically updated to reflect the committed changes.