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

Eric Liu via cfe-commits cfe-commits at lists.llvm.org
Mon May 30 13:23:40 PDT 2016


ioeric added inline comments.

================
Comment at: lib/Format/Format.cpp:1550
@@ +1549,3 @@
+  HeaderInsertionReplaces =
+      fixCppIncludeInsertions(Code, HeaderInsertionReplaces, Style);
+  NewReplaces.insert(HeaderInsertionReplaces.begin(),
----------------
djasper wrote:
> 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.
I really appreciate the way you structure it, which does make the code clearer. 

Just one coveat: `std::set_difference()` is not applicable on `Replacements` since it calls `std::copy()` on `Replacement`, which `Replacement` does not support.

I think another way to approach it would be to delete header insertion replacement and insert new replacements back if we can assume that there are only few header insertion replacements.  


http://reviews.llvm.org/D20734





More information about the cfe-commits mailing list