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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to