djasper marked 8 inline comments as done.
================
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)) {
----------------
klimek wrote:
> 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.
It's actually three lines I think. Also, I think we'll significantly extend the
logic here shortly (e.g. to properly support Google and LLVM styles), so I
don't think it is worth thinking about too much.
================
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;
----------------
klimek wrote:
> 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.
Done and removed the check inside sortIncludes.
================
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
----------------
klimek wrote:
> 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).
Maybe a class would be the way to go here. I'll try in a subsequent patch
(don't think it should be a priority at this point).
================
Comment at: lib/Tooling/Core/Replacement.cpp:341
@@ +340,3 @@
+ unsigned MergedEnd = MergedOffset + MergedText.size();
+ unsigned Offset = SecondI->getOffset() + DeltaSecond + Delta;
+ if (Offset > MergedEnd)
----------------
klimek wrote:
> If SecondI == Second.end(), isn't this an undefined access?
Cannot happen. The while starts with MergeSecond && SecondI != Second.end().
================
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;
----------------
klimek wrote:
> I'm surprised the explicit Twine() is needed here...
How else would this know to use a Twine for efficient concatenation?
http://reviews.llvm.org/D11240
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
