[PATCH] D20734: [clang-format] insert new #includes into correct blocks when cleaning up Replacement with cleanupAroundReplacements().

Daniel Jasper via cfe-commits cfe-commits at lists.llvm.org
Mon May 30 12:12:25 PDT 2016


djasper added inline comments.

================
Comment at: lib/Format/Format.cpp:1284
@@ +1283,3 @@
+  // if \p CheckMainHeader is true and \p IncludeName is a main header, returns
+  // 0. Otherwise, returns INT_MAX if \p IncludeName does not match any category
+  // pattern.
----------------
Nit: Otherwise, returns the priority of the matching category or INT_MAX.

================
Comment at: lib/Format/Format.cpp:1503
@@ +1502,3 @@
+  //  - to the beginning of the file.
+  for (auto Iter = ++Priorities.begin(), E = Priorities.end(); Iter != E;
+       ++Iter)
----------------
nit: I think you should be using "I" and "E".

================
Comment at: lib/Format/Format.cpp:1508
@@ +1507,3 @@
+      --Prev;
+      CategoryEndOffsets[*Iter] = CategoryEndOffsets[*Prev];
+    }
----------------
Can you just use "std::prev(I)" instead of Prev?

================
Comment at: lib/Format/Format.cpp:1550
@@ +1549,3 @@
+  HeaderInsertionReplaces =
+      fixCppIncludeInsertions(Code, HeaderInsertionReplaces, Style);
+  NewReplaces.insert(HeaderInsertionReplaces.begin(),
----------------
So, not knowing anything of it, what's the difference between fixCppIncludeInsertions and fixInsertionReplacements and when do you need to call what. To me the fact that it's that difficult to find a name for it is telling..

Also, I still don't understand why you think it is better to separate the two functions. In that way you are currently implementing it, it is certainly less efficient. You are always copying all the replacements, which (in the way you are doing it) is O(N lg N), even if there isn't a single header insertion. Maybe at least use set_difference (similar to what I am showing below).

So, how I would structure it:

- Pull out a function isHeaderInsertion(const Replacement &). Easy naming, good to pull this functionality out.
- At the start of fixCppIncludeInsertions, write:


```
    tooling::Replacements HeaderInsertions;
    for (const auto &R : Replaces)
      if (isHeaderInsertion(R))
        HeaderInsertions.insert(R);
    if (HeaderInsertions.empty())
      return Replaces;

    tooling::Replacements Result;
    std::set_difference(Replaces.begin(), Replaces.end(),
                        HeaderInsertions.begin(), HeaderInsertions.end(),
                        Result.begin());

```  
- Do what the function currently does with HeaderInsertions and add the results back to Result.


http://reviews.llvm.org/D20734





More information about the cfe-commits mailing list