klimek added a comment.
First round of comments; some things are still a bit confusing, so I hope
another round will help to weed them out.
================
Comment at: include/clang/Tooling/Core/Replacement.h:223-224
@@ -222,1 +222,4 @@
+/// \brief Merges to sets of replacements with the second set referring to the
+/// code after applying the first set.
+Replacements mergeReplacements(const Replacements &First,
----------------
s/to/two/
================
Comment at: lib/Format/Format.cpp:1614-1622
@@ +1613,11 @@
+
+ bool OutOfOrder = false;
+ for (unsigned i = 1, e = Indices.size(); i != e; ++i) {
+ if (Indices[i] != i) {
+ OutOfOrder = true;
+ break;
+ }
+ }
+ if (!OutOfOrder)
+ return;
+
----------------
I think this wants a comment along the lines of:
// We create one large replacement for the block of includes.
// If the order is already correct, we do not want to create that replacement.
================
Comment at: lib/Format/Format.cpp:1646
@@ +1645,3 @@
+ unsigned SearchFrom = 0;
+ llvm::Regex IncludeRegex(R"(^\ *#\ *include[^"<]*["<]([^">]*)([">]))");
+ SmallVector<StringRef, 4> Matches;
----------------
Do we want to use \s instead of "\ " so this also works for tab-using codebases?
================
Comment at: lib/Format/Format.cpp:1653
@@ +1652,3 @@
+ Code.substr(Prev, (Pos != StringRef::npos ? Pos : Code.size()) - Prev);
+ if (!Line.endswith("\\")) {
+ if (IncludeRegex.match(Line, &Matches)) {
----------------
I'd have expected early exit, but I can see it'd introduce a one-line
duplication. Not sure, just wanted to bring it up.
================
Comment at: lib/Format/Format.cpp:1662-1665
@@ +1661,6 @@
+ IncludesInBlock.push_back({Matches[1], Line, Prev, Matches[2] == ">"});
+ } else {
+ sortIncludes(Style, IncludesInBlock, Ranges, FileName, Replaces);
+ IncludesInBlock.clear();
+ }
+ Prev = Pos + 1;
----------------
Nit: I think this would be a bit more readable if we only call sortInlucdes if
!IncludesInBlock.empty() and add that as condition to the else.
================
Comment at: lib/Tooling/Core/Replacement.cpp:310-313
@@ +309,6 @@
+ FirstI != First.end() || SecondI != Second.end();) {
+ // 'MergeSecond' is true, if an element from 'Second' needs to be merged
+ // next, and false if an element from 'First' shoud be merged. As the input
+ // replacements are non-overlapping, we always need to merge an element
+ // from 'Second' into an element from 'First' or vice versa.
+ bool MergeSecond = SecondI == Second.end() ||
----------------
Nit: remove the first ','.
s/shoud/should/
Also: 'non-overlapping in each set'? (or is that incorrect? the function
comment in the header doesn't currently tell, I think we might want to change
that)
================
Comment at: lib/Tooling/Core/Replacement.cpp:314
@@ +313,3 @@
+ // from 'Second' into an element from 'First' or vice versa.
+ bool MergeSecond = SecondI == Second.end() ||
+ FirstI->getOffset() < SecondI->getOffset() + Delta;
----------------
Would be good to learn up here why SecondI == Second.end() implies "an element
from 'Second' needs to be merged".
Ok, it looks like this implies that the other one is the one that is the basis
for merging. Perhaps MergeIntoFirst?
================
Comment at: lib/Tooling/Core/Replacement.cpp:332-336
@@ +331,7 @@
+ // subsequent, non-overlapping replacements.
+ int DeltaFirst = MergeSecond ? MergedText.size() - MergedLength : 0;
+ int DeltaSecond = MergeSecond ? 0 : MergedText.size() - MergedLength;
+ while ((MergeSecond && SecondI != Second.end()) ||
+ (!MergeSecond && FirstI != First.end())) {
+ if (MergeSecond) {
+ // If we are merging an element from 'Second', the length of the
----------------
This structure looks like we'd want 2 functions. I assume you had reasons not
to pull out two local functions? What are they? Too many parameters? (I'd
probably pull out a class to be able to put this into smaller methods, but I'm
probably weird).
================
Comment at: lib/Tooling/Core/Replacement.cpp:341
@@ +340,3 @@
+ unsigned MergedEnd = MergedOffset + MergedText.size();
+ unsigned Offset = SecondI->getOffset() + DeltaSecond + Delta;
+ if (Offset > MergedEnd)
----------------
If SecondI == Second.end(), isn't this an undefined access?
================
Comment at: lib/Tooling/Core/Replacement.cpp:354
@@ +353,3 @@
+ StringRef Tail = MergedRef.substr(End - MergedOffset);
+ MergedText = (Twine(Head) + SecondI->getReplacementText() +
Tail).str();
+ DeltaSecond += SecondI->getReplacementText().size() - Length;
----------------
I'm surprised the explicit Twine() is needed here...
http://reviews.llvm.org/D11240
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
