Page MenuHomePhabricator

[include-fixer] Implement adding missing namespace qualifiers in vim integration.
ClosedPublic

Authored by hokein on Jul 13 2016, 3:06 AM.

Details

Summary

The patch extends include-fixer's "-output-headers", and "-insert-headers"
command line options to make it dump more information (e.g. QualifiedSymbol),
so that vim-integration can add missing qualifiers.

Diff Detail

Repository
rL LLVM

Event Timeline

hokein updated this revision to Diff 63788.Jul 13 2016, 3:06 AM
hokein retitled this revision from to [include-fixer] Implement adding missing namespace qualifiers in vim integration..
hokein updated this object.
hokein added a reviewer: bkramer.
hokein added a subscriber: cfe-commits.
hokein updated this revision to Diff 63789.Jul 13 2016, 3:09 AM

Remove a debug print statement.

bkramer added inline comments.Jul 13 2016, 3:45 AM
include-fixer/IncludeFixerContext.cpp
54 ↗(On Diff #63788)

Drop the const ...

57 ↗(On Diff #63788)

... use std::move to move Symbols into place.

include-fixer/IncludeFixerContext.h
32 ↗(On Diff #63788)

Typo: qulifiers

65 ↗(On Diff #63788)

s/informations/information/

include-fixer/tool/ClangIncludeFixer.cpp
252 ↗(On Diff #63788)

Is it guaranteed that two headers with the same name are adjacent in the HeaderInfos vector? If so, point that out in a comment, otherwise adjacent_find isn't the right algorithm here.

259 ↗(On Diff #63788)

"Expected exactly one unique header"?

include-fixer/tool/clang-include-fixer.py
119 ↗(On Diff #63788)

Reduplicate?

hokein updated this revision to Diff 63796.Jul 13 2016, 5:25 AM
hokein marked 6 inline comments as done.

Address review comments.

hokein marked an inline comment as done.Jul 13 2016, 5:26 AM
hokein added inline comments.
include-fixer/tool/ClangIncludeFixer.cpp
252 ↗(On Diff #63789)

You are right. adjancent_find is not correct algorithm here. What I want to do is to check all elements' headers in HeaderInfos are equal. Change to std::equal, the same below.

include-fixer/tool/clang-include-fixer.py
119 ↗(On Diff #63789)

Should be Deduplicate.

bkramer added inline comments.Jul 13 2016, 5:47 AM
include-fixer/tool/ClangIncludeFixer.cpp
252 ↗(On Diff #63796)

In that case adjacent_find was the right choice, but std::equal also works, I read this wrong. There are multiple ways of doing this using standard algorithms and none of them is really obvious. Please add a comment here that you're checking that all elements are equal.

hokein updated this revision to Diff 63799.Jul 13 2016, 6:02 AM
hokein marked 2 inline comments as done.

Add comments for std::equal.

include-fixer/tool/ClangIncludeFixer.cpp
252 ↗(On Diff #63796)

Done. To me, equal is more obvious than adjacent_find.

bkramer accepted this revision.Jul 13 2016, 6:04 AM
bkramer edited edge metadata.

One nit, LGTM.

include-fixer/tool/ClangIncludeFixer.cpp
277 ↗(On Diff #63799)

the same qualified name

This revision is now accepted and ready to land.Jul 13 2016, 6:04 AM
hokein updated this revision to Diff 63808.Jul 13 2016, 8:40 AM
hokein marked an inline comment as done.
hokein edited edge metadata.

Fix nits.

This revision was automatically updated to reflect the committed changes.