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

[Eric Liu via cfe-commits]

ioeric added inline comments.

================
Comment at: include-fixer/IncludeFixer.cpp:241
@@ +240,3 @@
+  IncludeFixerContext
+  GetIncludeFixerContext(const clang::SourceManager &SourceManager,
+                         clang::HeaderSearch &HeaderSearch) {
----------------
I think function name should start with lower case in llvm. 

================
Comment at: include-fixer/IncludeFixer.h:70
@@ +69,3 @@
+/// \return Replacements for inserting and sorting headers.
+std::vector<clang::tooling::Replacement> CreateReplacementsForHeader(
+    StringRef Code, StringRef FilePath, StringRef FallbackStyle,
----------------
Function name should start with lower case.

================
Comment at: include-fixer/tool/ClangIncludeFixer.cpp:211
@@ +210,3 @@
+
+  if (OuputHeaders) {
+    // FIXME: Output IncludeFixerContext as YAML.
----------------
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.

================
Comment at: include-fixer/tool/clang-include-fixer.py:49
@@ +48,3 @@
+                                                  default_choice_index)
+  return int(vim.eval(to_eval));
+
----------------
Maybe handle cases where `to_eval` is not a valid index?

================
Comment at: include-fixer/tool/clang-include-fixer.py:84
@@ +83,3 @@
+    index = 1;
+    for header in lines[1:]:
+      choices_message += "&" + str(index) + header + "\n"
----------------
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


http://reviews.llvm.org/D20621