This is an archive of the discontinued LLVM Phabricator instance.

[include-fixer] Create a mode in vim integration to show multiple potential headers.
ClosedPublic

Authored by hokein on May 25 2016, 6:58 AM.

Details

Summary

Some changes in the patch:

  • Add two commandline flags in clang-include-fixer.
  • Introduce a IncludeFixerContext for the queried symbol.
  • Pull out CreateReplacementsForHeader.

Diff Detail

Event Timeline

hokein updated this revision to Diff 58409.May 25 2016, 6:58 AM
hokein retitled this revision from to [include-fixer] Create a mode in vim integration to show multiple potential headers..
hokein added a reviewer: bkramer.
hokein updated this object.
hokein added subscribers: ioeric, cfe-commits.

Oh, sorry, I miss two separate commits here. This patch should not be ready for review. I need to rebase it after commit D20581.

hokein updated this revision to Diff 58420.May 25 2016, 7:43 AM

Fix a nit.

ioeric added inline comments.May 25 2016, 7:59 AM
include-fixer/IncludeFixer.cpp
241

This doesn't seem to be the right name. It does more than minimizing headers.

include-fixer/tool/ClangIncludeFixer.cpp
121

These comments seem redundant since it is already clear what those parameters are from variables' names.

129

This should only be for vim-mode or STDINMode I think. For normal mode, we should apply replacements on the rewriter.

208

Same here.

include-fixer/tool/clang-include-fixer.py
31

I think we can simply have one mode where a header is inserted if there is only one result, and a dialog is shown only if there are multiple headers for user to choose from.

31

Also remove semicolons here and below...

78

I think we should also update the buffer with the diff method below. It doesn't make sense to clear the buffer if we are just inserting one line.

hokein updated this revision to Diff 58448.May 25 2016, 10:04 AM
hokein marked 6 inline comments as done.

Update and address comments.

bkramer edited edge metadata.May 25 2016, 10:07 AM

Can you add some lit tests for the various command line modes clang-include-fixer has now. We can't reasonably test the vim integration but we can tests the bits it's composed of.

bkramer added inline comments.May 25 2016, 10:12 AM
include-fixer/IncludeFixer.h
51–52

maybe 'about the symbol being queried'?

72

'Header' looks like it should be a StringRef too.

include-fixer/IncludeFixerContext.h
20

I think some doxygen comments would be in order for this struct and its members.

include-fixer/tool/ClangIncludeFixer.cpp
62

typo: Ouput

ioeric added inline comments.May 25 2016, 12:28 PM
include-fixer/IncludeFixer.cpp
243

I think function name should start with lower case in llvm.

include-fixer/IncludeFixer.h
70

Function name should start with lower case.

include-fixer/tool/ClangIncludeFixer.cpp
217

I'd put this before creating replacements and right after you get the context since those replacements are not used here. And the "Added #include..." message above doesn't make sense in this mode.

include-fixer/tool/clang-include-fixer.py
49

Maybe handle cases where to_eval is not a valid index?

72

If there is only one candidate, it doesn't make sense to ask users to choose it IMO. We can simply insert the header in this case.

And I think we should make users pick the correct header if there are multiple candidates by default. What do you think, Ben? @bkramer

bkramer added inline comments.May 25 2016, 12:31 PM
include-fixer/tool/clang-include-fixer.py
72

I agree, we should ask by default when there is more than one suggested header.

hokein updated this revision to Diff 58941.May 30 2016, 3:10 AM
hokein marked 9 inline comments as done.
hokein edited edge metadata.

Address comments.

Can you add some lit tests for the various command line modes clang-include-fixer has now. We can't reasonably test the vim integration but we can tests the bits it's composed of.

Done.

include-fixer/tool/clang-include-fixer.py
49

vim can handle this for us.

hokein updated this revision to Diff 58942.May 30 2016, 3:12 AM

Remove unneeded headers.

klimek added a subscriber: klimek.May 30 2016, 3:50 AM
klimek added inline comments.
include-fixer/IncludeFixer.cpp
398

Do we want UINT_MAX? I find the wording in the standard to be too involved for me to easily understand that this is both well-defined and what we want and portable.

include-fixer/IncludeFixer.h
62–72
  1. This only needs one style, so I think we should pass it in instead of FilePath and FallbackStyle
  2. From the docs it seems like FirstIncludeOffset should always be INT_MAX? Can we just leave out that parameter and make the function do the right thing?
  3. Generally, I'd order parameters by "importance", that is, the ones that are core to the functionality go first (C++ kinda implies this by only allowing later parameters being defaultable); so basically, I'd make this:

    .. insertHeader(StringRef Code, StringRef Header, const FormatStyle& Style);
hokein updated this revision to Diff 58955.May 30 2016, 7:28 AM
hokein marked an inline comment as done.

Refactor createReplacementsForHeaders.

ioeric added inline comments.May 30 2016, 7:45 AM
include-fixer/IncludeFixer.h
79

I don't see why we'd want Style to be optional.

include-fixer/tool/ClangIncludeFixer.cpp
111

We might still want to use clang::format::getStyle("file", FilePath, FallbackStyle);, which will use format style defined in clang-format config file in the path, if such config file exists. Style option is just a fallback style.

hokein updated this revision to Diff 58964.May 30 2016, 8:39 AM
hokein marked an inline comment as done.

Use format::getStyle to get clang-format style.

hokein added inline comments.May 30 2016, 8:54 AM
include-fixer/IncludeFixer.h
79

Using a default argument in Style can simplify the code in some cases, for example in the Unittest. @klimek might have idea?

bkramer added inline comments.May 30 2016, 10:04 AM
include-fixer/IncludeFixer.h
70

I think you can drop the part about 'uninitialization' from this comment.

78

This doesn't look clang-formatted :p

hokein updated this revision to Diff 59018.May 31 2016, 12:49 AM
hokein marked 2 inline comments as done.

Fix code style.

bkramer accepted this revision.May 31 2016, 1:51 AM
bkramer edited edge metadata.

LG. Can't wait to use it myself :)

This revision is now accepted and ready to land.May 31 2016, 1:51 AM
hokein updated this revision to Diff 59026.May 31 2016, 2:20 AM
hokein edited edge metadata.

Update a out-of-date comment.

klimek added inline comments.May 31 2016, 2:22 AM
include-fixer/IncludeFixer.h
73

This is still missing documentation on what the -1U special case means.

hokein updated this revision to Diff 59028.May 31 2016, 2:33 AM

Add -1U comment back.

hokein marked an inline comment as done.May 31 2016, 2:34 AM
klimek accepted this revision.May 31 2016, 2:38 AM
klimek added a reviewer: klimek.

lg

This revision was automatically updated to reflect the committed changes.

LG. Can't wait to use it myself :)

Currently, the header is only inserted at the first line of the file because we don't output the FirstIncludeOffset to py script. A follow-up patch will come soon.