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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
