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

Manuel Klimek via cfe-commits cfe-commits at lists.llvm.org
Mon May 30 03:50:37 PDT 2016


klimek added a subscriber: klimek.

================
Comment at: include-fixer/IncludeFixer.cpp:397
@@ +396,3 @@
+  clang::tooling::Replacements Insertions;
+  if (FirstIncludeOffset == -1U) {
+    // FIXME: skip header guards.
----------------
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.

================
Comment at: include-fixer/IncludeFixer.h:62-72
@@ -64,1 +61,13 @@
 
+/// Insert a header before the first #include in \p Code and run
+/// clang-format to sort all headers.
+/// \param Code The source code.
+/// \param FilePath The source file path.
+/// \param StyleName Fallback style for reformatting.
+/// \param FirstIncludeOffset The insertion point for new include directives.
+/// \param Header The header being inserted.
+/// \return Replacements for inserting and sorting headers.
+std::vector<clang::tooling::Replacement> createReplacementsForHeader(
+    StringRef Code, StringRef FilePath, StringRef FallbackStyle,
+    unsigned FirstIncludeOffset, StringRef Header);
+
----------------
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);


http://reviews.llvm.org/D20621





More information about the cfe-commits mailing list