Added calculateRangesAfterReplaments() to calculate original ranges as well as
newly affacted ranges in the new code.
Details
- Reviewers
klimek djasper - Commits
- rG8b636db8d4c6: Added calculateRangesAfterReplaments() to calculate affacted ranges in the new…
rC273290: Added calculateRangesAfterReplaments() to calculate affacted ranges in the new…
rL273290: Added calculateRangesAfterReplaments() to calculate affacted ranges in the…
Diff Detail
- Repository
- rL LLVM
Event Timeline
include/clang/Tooling/Core/Replacement.h | ||
---|---|---|
68 ↗ | (On Diff #61337) | Can we keep these private for now? |
237 ↗ | (On Diff #61337) | Should we make the latter optional? |
240 ↗ | (On Diff #61337) | s/Replacemments/Replacements/ |
245 ↗ | (On Diff #61337) | I'd call this getShiftedRanges(). |
246 ↗ | (On Diff #61337) | const vector&? |
lib/Tooling/Core/Replacement.cpp | ||
39 ↗ | (On Diff #61337) | No need for parentheses. |
304 ↗ | (On Diff #61337) | Remove " a" |
305 ↗ | (On Diff #61337) | Just return the new vector and use a const parameter. |
328 ↗ | (On Diff #61337) | Couldn't we implement this using mergeReplacements? Fundamentally:
|
include/clang/Tooling/Core/Replacement.h | ||
---|---|---|
253 ↗ | (On Diff #61359) | getShiftedRanges sounds like it's excluding affected ranges of Replaces. I think often time we want both? |
237 ↗ | (On Diff #61340) | Why would why want to exclude the newly affected ranges? But if replaces overlap with original ranges, do we consider sub-ranges in original ranges being replaced as parts of the shifted ranges in the new code? |
lib/Tooling/Core/Replacement.cpp | ||
305 ↗ | (On Diff #61340) | Shall we pass Ranges by value instead since it's gonna be sorted anyway? Also, does it make sense to have calculateChangedRanges() call this? |
328 ↗ | (On Diff #61340) | That actually works very well! Thanks! |
Nice :)
include/clang/Tooling/Core/Replacement.h | ||
---|---|---|
62 ↗ | (On Diff #61359) | Can we just add a lambda that does this to the std::sort operation? I am not sure that this is actually a natural order for all Replacements. |
69 ↗ | (On Diff #61359) | Why is this required? |
lib/Tooling/Core/Replacement.cpp | ||
287 ↗ | (On Diff #61359) | maybe: std::vector<Range> Result; for (const auto &R: Ranges) { if (Result.empty() || !Result.back().overlapsWith(R)) { Result.push_back(R); } else { unsigned NewEnd = std::max(Result.back.getOffset() + Result.back.getLength(), R.getOffset() + R.getLength()); Result[Result.size() - 1] = Range(Result.back.getOffset(), NewEnd - Result.back.getOffset()); } } |
319 ↗ | (On Diff #61359) | I'd probably leave these early exits out. I don't think anyone will ever notice the performance difference. |
321 ↗ | (On Diff #61359) | I think you need to do: Ranges = mergeAndSortRanges(Ranges); here |
- Addressed reviewer's comments.
include/clang/Tooling/Core/Replacement.h | ||
---|---|---|
69 ↗ | (On Diff #61359) | It is used in unit test to test the equality between two ranges. |
lib/Tooling/Core/Replacement.cpp | ||
287 ↗ | (On Diff #61359) | Thanks! Just one minor point: we also want to merge ranges R1 and R2 if R1.getOffset() + R1.getLength() == R2.getOffset() For example, if we have R1(1, 1) and R2(2, 1), then we can merge them into R3(1, 2). |
A couple of small comments, otherwise looks good.
lib/Tooling/Core/Replacement.cpp | ||
---|---|---|
284 ↗ | (On Diff #61375) | I don't think you need "-> bool". |
285 ↗ | (On Diff #61375) | or std::tie(LHS.getOffset(), LHS.getLength()) < std::tie(RHS.getOffset(), RHS.getLength())? |
292 ↗ | (On Diff #61375) | Maybe pull out: unsigned CurrentEnd = Result.back().getOffset() + Result.back().getLength(); Will probably save a line or two :) |